generative-ai-python icon indicating copy to clipboard operation
generative-ai-python copied to clipboard

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

Open Faisal-Alsrheed opened this issue 1 year ago • 1 comments

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 avatar May 14 '24 18:05 Faisal-Alsrheed

Hi @MarkDaoust :)

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

Faisal-Alsrheed avatar May 16 '24 16:05 Faisal-Alsrheed

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 :)

mayureshagashe2105 avatar May 17 '24 03:05 mayureshagashe2105

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 avatar May 17 '24 18:05 MarkDaoust