fix: validate provider urls before use #147
Conversation
| key = os.environ.get("OPENAI_API_KEY") | ||
|
|
||
| client = httpx.Client( | ||
| base_url=url + "v1/", | ||
| base_url=url.join("v1/"), |
There was a problem hiding this comment.
fyi using httpx.URL as it isn't so sensitive about trailing slash etc when joining
There was a problem hiding this comment.
interesting, i always remembered // would resolve? just curious what bug you can into since i haven't seen this before!
There was a problem hiding this comment.
I think it was more about not having a slash at all. e.g if you set a base URL ending in the port
|
@lamchau can you cast eagle eyes over this? |
|
|
||
| val = os.environ.get(key, default) | ||
| if val == "": | ||
| raise ValueError(f"{key} was empty") |
There was a problem hiding this comment.
:nit: should use KeyError which is consistent with the non-default value getter os.environ[key].
[nav] In [1]: import os
...:
...: os.environ["SHELL"]
...: os.environ["SHELL_MISSING"]
...:
---------------------------------------------------------------------------
KeyError Traceback (most recent call last)
Cell In[1], line 4
1 import os
3 os.environ["SHELL"]
----> 4 os.environ["SHELL_MISSING"]
File <frozen os>:714, in __getitem__(self, key)
KeyError: 'SHELL_MISSING'There was a problem hiding this comment.
forgot what package i was in – it just occurred to me that we recently wrote up a way to do this on the base provider. could we add the check in there?
There was a problem hiding this comment.
so you mean add a parameter to check_env_vars() like base_url_key?
There was a problem hiding this comment.
exactly! imo we might want to tweak it slightly so it'll dump all the env vars in a single pass rather than one at a time (especially with how much setup is needed for AZURE_*).
not sure if @elenazherdeva is working on that atm, but it was from this #116 (comment)
There was a problem hiding this comment.
@lamchau thank you for pinging! I’m not working on it at the moment, but I should fix it. Let me create a follow-up PR based on your comments!
|
thanks for the advice @lamchau will revise impl after clarification on #147 (comment) |
|
sorry was awol on medical leave. sporadic this week, but will adjust this and others soon |
|
@codefromthecrypt no worries! hopefully you're doing well/on the mend! |
Signed-off-by: Adrian Cole <adrian.cole@elastic.co>
codefromthecrypt
force-pushed
the
validate_env
branch
from
4dc27d9
to
224d729
Compare
codefromthecrypt
force-pushed
the
validate_env
branch
from
224d729
to
e8049a4
Compare
|
@lamchau PTAL I have consolidated what I could into the same check, and refactored things around it. |
| @@ -164,5 +164,5 @@ def recommended_models() -> tuple[str, str]: | |||
|
|
|||
| @retry_procedure | |||
| def _post(self, payload: dict) -> httpx.Response: | |||
| response = self.client.post(ANTHROPIC_HOST, json=payload) | |||
| response = self.client.post(self.BASE_URL_DEFAULT, json=payload) | |||
There was a problem hiding this comment.
I think this is incorrect as it defers to the default value without considering an override. OTOH, if I make this empty, it fails tests...
| REQUIRED_ENV_VARS = [ | ||
| "AZURE_CHAT_COMPLETIONS_HOST_NAME", |
There was a problem hiding this comment.
I separated out the base URL enforcement from the other ENV vars as there is special handling
| @@ -24,6 +21,8 @@ class GoogleProvider(Provider): | |||
| """Provides chat completions for models hosted by Google, including Gemini and other experimental models.""" | |||
|
|
|||
| PROVIDER_NAME = "google" | |||
| BASE_URL_ENV_VAR = "GOOGLE_HOST" | |||
| BASE_URL_DEFAULT = "https://generativelanguage.googleapis.com/v1beta" | |||
There was a problem hiding this comment.
does it make sense to have the constant at the top of the file just as people may want to quickly scan/change?
There was a problem hiding this comment.
So, these constants are accessed as class variables, so if we move to the top they'd no longer be that. I'm not sure a hack to work around this..
This fixes validation, like #133, except for all providers that accept a hostname/base URL variable.