-
Notifications
You must be signed in to change notification settings - Fork 406
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
Conversation
get_model
, get_base_model
, get_tuned_model
get_model
, get_base_model
, get_tuned_model
, _make_grounding_passages
, _make_generate_answer_request
get_model
, get_base_model
, get_tuned_model
, _make_grounding_passages
, _make_generate_answer_request
generative_models.py
, models.py
, answer.py
, client.py
, and types/permission_types.py
generative_models.py
, models.py
, answer.py
, client.py
, and types/permission_types.py
generative_models.py
, models.py
, answer.py
, client.py
, and types/permission_types.py
generative_models.py
, models.py
, answer.py
, client.py
, and types/permission_types.py
Hi @MarkDaoust :) Let me know if you would like me to split this into multiple PRs. |
There was a problem hiding this 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!
There was a problem hiding this 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
The one in the permission_types? I should submit it in a separate PR. @Faisal-Alsrheed thank you for the typos and other fixes :) |
Change-Id: I3ad717ca26e5d170e4bbef23076e528badaaaacb
No I meant the one at the end of the PR description. I think this is good to submit. |
General Updates
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.
Typo Corrections: Fixed typos in comments and function names, notably correcting
get_dafault_permission_client
toget_default_permission_client
and its asynchronous counterpart inclient.py
andtypes/permission_types.py
. These changes prevent potential bugs and improve overall code quality.Specific File Changes
generative_models.py
:'models/gemini-pro'
), aligning with changes inmodels.py
.models.py
:tunedModels/
from theget_model
function. Now, model fetching is uniformly handled under themodels/
prefix, reducing complexity.models/
ortunedModels/
), aiding in quicker debugging and error resolution.answer.py
:client.py
:get_dafault_permission_client
toget_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
:notebook/text_model_test.py
:unittest
vspytest
I noticed discrepancies in the number of tests discovered when using
unittest
andpytest
.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
innotebook/text_model_test.py
for better debugging.beacuse after using
pytest
i got thisThe
call_model
method of theTestModel
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! 😊