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

Enable base_url to read from env variables #65

Merged
merged 13 commits into from
Jul 30, 2024

Conversation

kylehh
Copy link
Contributor

@kylehh kylehh commented Jul 8, 2024

Add root_validator in class ChatNVIDIA to enable base_url read from the env variable
The logic order the validation
1, check nvidia_base_url argument in ChatNVIDIA class
2, check base_url argument in ChatNVIDIA class
3, check NVIDIA_BASE_URL environment variable
4, use the default value defined in _default_base_url

@mattf mattf requested a review from raspawar July 9, 2024 08:34
@raspawar
Copy link
Collaborator

raspawar commented Jul 9, 2024

@kylehh This change is expected for all nvidia constructors(embedding, ranking etc.) right?
also implementation like api_key parameter is expected I guess(changes in _common.py) with passing it in _NVIDIAClient() constructor.
Corresponding changes in test cases, notebook is necessary.

@mattf
Copy link
Collaborator

mattf commented Jul 9, 2024

welcome @kylehh!

in addition to Rashmi's comments, let's skip the nvidia_base_url argument option, if people are passing the base url as an argument they can use the existing base_url arg.

@kylehh
Copy link
Contributor Author

kylehh commented Jul 12, 2024

@kylehh This change is expected for all nvidia constructors(embedding, ranking etc.) right? also implementation like api_key parameter is expected I guess(changes in _common.py) with passing it in _NVIDIAClient() constructor. Corresponding changes in test cases, notebook is necessary.

Thanks for reviewing @raspawar yes, I can add similar logic in embedding and reranking. api_key can already read in from env variable ( implemented in _common.py). Will add test case as well

@kylehh
Copy link
Contributor Author

kylehh commented Jul 12, 2024

welcome @kylehh!

in addition to Rashmi's comments, let's skip the nvidia_base_url argument option, if people are passing the base url as an argument they can use the existing base_url arg.
Hi @mattf
api_key can be read in using both nvidia_api_key and api_key arguments. I think it's fine to keep this logic, and nvidia_ prefix argument is not explicitly defined, but take in from kwargs

@kylehh
Copy link
Contributor Author

kylehh commented Jul 18, 2024

I added base_url logic to embedding and rerank. I see @mattf had a PR for base_url validation. Can we merge both PR and then I will see if I need extra tests for base_url

Copy link
Collaborator

@mattf mattf left a comment

Choose a reason for hiding this comment

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

please add unit tests for NVIDIA_BASE_URL env var, you can do the same as what is in test_api_key.py

@mattf mattf merged commit f1cd9eb into langchain-ai:main Jul 30, 2024
12 checks passed
@mattf mattf deleted the add-base-url-env branch July 30, 2024 18:13
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.

3 participants