Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs, improve code clarity, and enhance overall reliability across several files. #339

Merged
merged 12 commits into from
May 17, 2024

Conversation

Faisal-Alsrheed
Copy link
Contributor

@Faisal-Alsrheed Faisal-Alsrheed commented May 14, 2024

General Updates

  1. Documentation and Error Messages: Improved clarity and accuracy in documentation and error messages across multiple files. This includes providing more descriptive error messages and consistent usage examples to help developers better understand code functionalities and constraints.

  2. Typo Corrections: Fixed typos in comments and function names, notably correcting get_dafault_permission_client to get_default_permission_client and its asynchronous counterpart in client.py and types/permission_types.py. These changes prevent potential bugs and improve overall code quality.

Specific File Changes

  • generative_models.py:

    • Corrected a typo from "equvalents" to "equivalents".
    • Updated example usage to use a consistent model path format ('models/gemini-pro'), aligning with changes in models.py.
  • models.py:

    • Simplified functionality by removing handling of tunedModels/ from the get_model function. Now, model fetching is uniformly handled under the models/ prefix, reducing complexity.
    • Updated error messages to include the problematic name input when model names do not start with required prefixes (models/ or tunedModels/), aiding in quicker debugging and error resolution.
  • answer.py:

    • Improved error message clarity to aid developers in understanding the requirements and restrictions in data handling.
    • Enhanced function documentation to provide clearer instructions on their usage and the expected inputs.
  • client.py:

    • Corrected spelling mistakes in function names from get_dafault_permission_client to get_default_permission_client and similarly for the async client functions. This correction is critical for preventing potential misconfigurations and ensuring the accuracy of function calls.
  • types/permission_types.py:

    • Fixed typos in function names and modified the functions to use the corrected names, enhancing accuracy and preventing potential misconfigurations.
    • Streamlined permission handling processes to enhance overall system reliability.
  • notebook/text_model_test.py:

    • I have broken down test_generate_text for better debugging (The call_model method of the TestModel class seems to be returning these values as strings, which is causing some tests to fail)

unittest vs pytest

I noticed discrepancies in the number of tests discovered when using unittest and pytest.

When running our test suite with unittest using the command:

python -m unittest discover -s /workspaces/generative-ai-python/tests
unittest reported that it ran 603 tests.

However, when we ran the same test suite with pytest using the command:

pytest /workspaces/generative-ai-python/tests
pytest reported that it collected and ran 607 tests.

TODO:

I have broken down test_generate_text in notebook/text_model_test.py for better debugging.
beacuse after using pytest i got this

======================================================================================== short test summary info ========================================================================================
FAILED tests/notebook/text_model_test.py::TextModelTestCase::test_generate_text_with_args_candidate_count - AssertionError: '5' != 5
FAILED tests/notebook/text_model_test.py::TextModelTestCase::test_generate_text_with_args_temperature - AssertionError: '0.42' != 0.42
FAILED tests/notebook/text_model_test.py::TextModelTestCase::test_generate_text_without_args_none_results - AssertionError: 'None' is not None
============================================================================== 3 failed, 609 passed, 35 warnings in 5.15s ===============================================================================

The call_model method of the TestModel class seems to be returning these values as strings, which is causing the tests to fail.

Please review the changes and provide any additional feedback.

Thank You! 😊

@Faisal-Alsrheed Faisal-Alsrheed marked this pull request as ready for review May 14, 2024 18:07
@github-actions github-actions bot added status:awaiting review PR awaiting review from a maintainer component:python sdk Issue/PR related to Python SDK labels May 14, 2024
@MarkDaoust MarkDaoust self-requested a review May 14, 2024 19:20
@Faisal-Alsrheed Faisal-Alsrheed changed the title Fix and improve get_model, get_base_model, get_tuned_model Fix and improve get_model, get_base_model, get_tuned_model , _make_grounding_passages , _make_generate_answer_request May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed changed the title Fix and improve get_model, get_base_model, get_tuned_model , _make_grounding_passages , _make_generate_answer_request Fix and improve in generative_models.py, models.py, answer.py, client.py, and types/permission_types.py May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed changed the title Fix and improve in generative_models.py, models.py, answer.py, client.py, and types/permission_types.py Fix and improve generative_models.py, models.py, answer.py, client.py, and types/permission_types.py May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed marked this pull request as draft May 15, 2024 13:06
@Faisal-Alsrheed Faisal-Alsrheed changed the title Fix and improve generative_models.py, models.py, answer.py, client.py, and types/permission_types.py fixing bugs, improving code clarity, and enhancing overall reliability across several files. May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed changed the title fixing bugs, improving code clarity, and enhancing overall reliability across several files. Fixing bugs, improving code clarity, and enhancing overall reliability across several files. May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed changed the title Fixing bugs, improving code clarity, and enhancing overall reliability across several files. fix: correct typos, enhance documentation, and simplify code across multiple modules May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed changed the title fix: correct typos, enhance documentation, and simplify code across multiple modules Fix bugs, improve code clarity, and enhance overall reliability across several files. May 15, 2024
@Faisal-Alsrheed Faisal-Alsrheed marked this pull request as ready for review May 16, 2024 16:41
@Faisal-Alsrheed
Copy link
Contributor Author

Hi @MarkDaoust :)

Let me know if you would like me to split this into multiple PRs.

MarkDaoust
MarkDaoust previously approved these changes May 17, 2024
Copy link
Collaborator

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

This is great. Thank you!

Change-Id: I4e222f3e01cb8d350ae293b35a88fd5f718fe3dc
Copy link
Collaborator

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

Wait I need to look at that TODO before merging this.

Edit: resolved

@mayureshagashe2105
Copy link
Contributor

Wait I need to look at that TODO before merging this.

The one in the permission_types? I should submit it in a separate PR.

@Faisal-Alsrheed thank you for the typos and other fixes :)

CONTRIBUTING.md Outdated Show resolved Hide resolved
Change-Id: I3ad717ca26e5d170e4bbef23076e528badaaaacb
CONTRIBUTING.md Outdated Show resolved Hide resolved
@MarkDaoust
Copy link
Collaborator

The one in the permission_types? I should submit it in a separate PR.

No I meant the one at the end of the PR description.

I think this is good to submit.

@MarkDaoust MarkDaoust merged commit 51d806d into google-gemini:main May 17, 2024
7 checks passed
@github-actions github-actions bot removed the status:awaiting review PR awaiting review from a maintainer label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:python sdk Issue/PR related to Python SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants