langchain-ai / langchain-nvidia

MIT License
50 stars 16 forks source link

Enable base_url to read from env variables #65

Closed kylehh closed 2 months ago

kylehh commented 3 months ago

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

raspawar commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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