Skip to content

Conversation

@Mushmou
Copy link
Owner

@Mushmou Mushmou commented Aug 1, 2023

No description provided.

rubynguyen1510 and others added 15 commits July 27, 2023 15:54
Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
* finalized main.py

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
* Fixed test main

* Working version of Text To Speech

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
* Fixed test main

* Working version of Text To Speech

* Deleted aws sandbox dir

* made a working version of google test and fixed run commands in req.txt

* Working version of azure and google, made change to readme, removed azure from req

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
* Fixed test main

* Working version of Text To Speech

* Deleted aws sandbox dir

* made a working version of google test and fixed run commands in req.txt

* Working version of azure and google, made change to readme, removed azure from req

* removed test file and added ok check for main.py

* working version of main.py and updated a test in testmain for aws

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
All 3 providers working in main.py, only missing azure test now.
* Fixed test main

* Working version of Text To Speech

* Deleted aws sandbox dir

* made a working version of google test and fixed run commands in req.txt

* Working version of azure and google, made change to readme, removed azure from req

* removed test file and added ok check for main.py

* working version of main.py and updated a test in testmain for aws

* All three services working in main.py and fixed key error in test main

* Finished test in unittest

* fixed a small space

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
Co-authored-by: Noah Jacinto <noahjacinto@Noahs-Air.fios-router.home>
fixed linting errors main.py
* Fixed test main

* Working version of Text To Speech

* Deleted aws sandbox dir

* made a working version of google test and fixed run commands in req.txt

* Working version of azure and google, made change to readme, removed azure from req

* removed test file and added ok check for main.py

* working version of main.py and updated a test in testmain for aws

* All three services working in main.py and fixed key error in test main

* Finished test in unittest

* fixed a small space

* finished test main

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
Co-authored-by: Noah Jacinto <noahjacinto@Noahs-Air.fios-router.home>

Your function is now listening on port `3000`, and you can execute it by sending `POST` request with appropriate authorization headers. To learn more about runtime, you can visit Python runtime [README](https://github.com/open-runtimes/open-runtimes/tree/main/runtimes/python-3.10).

## 📝 Notes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the README is still a TODO?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we are working on this.

def test_speech_invalid_credential(self) -> None:
"""Test credentials for speech method."""
instance = self.get_azure_instance("WRONG_API_KEY", "WRONG_PROJECT_ID")
# Mock the requests.post method used in get_token.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to avoid the temptation to write comments which says the same thing as the code.

Comments are great for explaining why you are doing something or for explaining complicated code. For simple code, though, don't add comments which just repeat the code.

Comment on lines 264 to 265
def get_req(self, payload: str, variables: str, provider: str, text: str,
language: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def get_req(self, payload: str, variables: str, provider: str, text: str,
language: str) -> None:
def get_req(
self,
payload: str,
variables: str,
provider: str,
text: str,
language: str,
) -> None:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using all of these arguments?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we did use all the args in the parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to remove this file?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we intend to remove .gitignore

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot remove it now because every time we run the code, py_cache is generated. When we are done with testing and preparing to make a pull request, we can remove the .gitignore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It looks like you removed the repo-wide python/.gitignore file and added a custom python/text-to-speech/,gitignore file. You probably should not remove the former. Edits to that file should be out of scope for this PR.
  2. You should be able to git commit without using the --all flag and specifying which files are or are not included in a commit.

"""
# Endpoint for cognitive services speech api
url = (
f"https://{Azure.REGION}.tts."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"https://{Azure.REGION}.tts."
f"https://{self.REGION}.tts."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain the difference between the two?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say someone decided to implement an AzureAsia class ... which is exactly like Azure except it uses a different region.

class AzureAsia(Azure):
    REGION = "asia"

If you read the region from self.REGION then the AzureAsia class will read the correct region. However, if you're reading the Azure.REGION then the updated REGION value will be ignored.

# Raise Exception.
mock_synthesize_speech.side_effect = Exception
# Assert the raise.
self.assertRaises(Exception, instance.speech, "hello", "en-EN")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the instance.speech call be inside the mock context?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt notice we can remove the instance.speech, I thought we had to keep the function and its params but I removed it. The change only includes the patch with raise exception.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update we changed the code again.

    def test_speech_invalid_language(self) -> None:
        """Test language for speech method."""
        # Set up mock.
        instance = self.get_google_instance("123", "123")
        with mock.patch.object(
            texttospeech.TextToSpeechClient, "synthesize_speech"
        ) as mock_synthesize_speech:
            # Raise Exception.
            mock_synthesize_speech.side_effect = Exception
            # Assert the raise.
            self.assertRaises(Exception, instance.speech, "hello", "en-EN")

How does this look?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better :) This code calls speech() with a mocked client, which I believe was the intent.

* Fixed test main

* Working version of Text To Speech

* Deleted aws sandbox dir

* made a working version of google test and fixed run commands in req.txt

* Working version of azure and google, made change to readme, removed azure from req

* removed test file and added ok check for main.py

* working version of main.py and updated a test in testmain for aws

* All three services working in main.py and fixed key error in test main

* Finished test in unittest

* fixed a small space

* finished test main

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
Co-authored-by: Noah Jacinto <noahjacinto@Noahs-Air.fios-router.home>
@Mushmou Mushmou requested a review from rubynguyen1510 August 2, 2023 21:41
Mushmou pushed a commit that referenced this pull request Aug 2, 2023
* Fixed test main

* Working version of Text To Speech

* Deleted aws sandbox dir

* made a working version of google test and fixed run commands in req.txt

* Working version of azure and google, made change to readme, removed azure from req

* removed test file and added ok check for main.py

* working version of main.py and updated a test in testmain for aws

* All three services working in main.py and fixed key error in test main

* Finished test in unittest

* fixed a small space

* finished test main

* added the changes from PR #20

---------

Co-authored-by: Noah Jacinto <noahjacinto@Noahs-MacBook-Air.local>
Co-authored-by: Noah Jacinto <noahjacinto@Noahs-Air.fios-router.home>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It looks like you removed the repo-wide python/.gitignore file and added a custom python/text-to-speech/,gitignore file. You probably should not remove the former. Edits to that file should be out of scope for this PR.
  2. You should be able to git commit without using the --all flag and specifying which files are or are not included in a commit.

# Raise Exception.
mock_client.side_effect = Exception
# Assert the raise.
self.assertRaises(Exception, instance.speech, None, "en-US")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this call the speech() function with a mocked or real client?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we were supposed to put the assert raise with the with block, so it can be part of the mocked client.

"""
# Endpoint for cognitive services speech api
url = (
f"https://{Azure.REGION}.tts."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say someone decided to implement an AzureAsia class ... which is exactly like Azure except it uses a different region.

class AzureAsia(Azure):
    REGION = "asia"

If you read the region from self.REGION then the AzureAsia class will read the correct region. However, if you're reading the Azure.REGION then the updated REGION value will be ignored.

# Raise Exception.
mock_synthesize_speech.side_effect = Exception
# Assert the raise.
self.assertRaises(Exception, instance.speech, "hello", "en-EN")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better :) This code calls speech() with a mocked client, which I believe was the intent.

modified main.py
# and can be added to the global gitignore or merged into this file. For a more nuclear
# option (not recommended) you can uncomment the following to ignore the entire idea folder.
#.idea/
#.idea/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be changing as part of this PR

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our plan is to add the changes to a new branch called feat 4122 text to speech branch, it would fix this I assume.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we create the feat-4155 with only one commit, we will double-check to ensure that changes in this file do not appear.

```json
{
"success":false,
"error":"Missing API_KEY""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub highlighting is telling me this is invalid JSON

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay we fixed the extra quote on README.md

polly_client = boto3.Session(
aws_access_key_id=self.api_key,
aws_secret_access_key=self.secret_api_key,
region_name=self.REGION).client("polly")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
region_name=self.REGION).client("polly")
region_name=self.REGION,
).client("polly")

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this one.

provider_class = Google(req)
elif provider == "azure":
provider_class = Azure(req)
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to check the provider string here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already check if the provider is in the list of Supported_providers in the validate_common function (line 241 - 243).

def __init__(
self,
data: dict[dict[str, str, str], dict[str, str]],
) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
) -> None:
) -> None:

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay we fixed this one.

Mushmou added 2 commits August 3, 2023 11:50
modified main
fix gitignore.
Mushmou added 2 commits August 3, 2023 15:29
fixed main
@IsaacG IsaacG closed this Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants