bug: honor PORTKEY_BASE_URL env var in set_base_url#435
Open
anxkhn wants to merge 1 commit into
Open
Conversation
set_base_url passed the PORTKEY_BASE_URL URL value constant
("https://api.portkey.ai/v1") to os.environ.get instead of the
env-var name, so it looked up an environment variable literally
named "https://api.portkey.ai/v1" (always None). As a result the
PORTKEY_BASE_URL environment variable was silently ignored and the
client fell through to the localhost default (or the public cloud
URL when an api_key was present), sending requests to the wrong host
for anyone pointing PORTKEY_BASE_URL at a self-hosted gateway.
Add a PORTKEY_BASE_URL_ENV = "PORTKEY_BASE_URL" name constant,
matching the existing PORTKEY_API_KEY_ENV / PORTKEY_PROXY_ENV
convention, and read it in set_base_url. Precedence is unchanged:
explicit base_url arg > PORTKEY_BASE_URL env > PORTKEY_PROXY env >
api_key-gated default. Add offline regression tests for set_base_url.
There was a problem hiding this comment.
Pull request overview
Fixes set_base_url so it correctly reads the PORTKEY_BASE_URL environment variable (by using an env-var-name constant rather than the base URL value), aligning behavior with the documented/env-based configuration flow and preventing silent fall-through to unintended hosts.
Changes:
- Add
PORTKEY_BASE_URL_ENV = "PORTKEY_BASE_URL"to centralize the env-var name alongside existing*_ENVconstants. - Update
set_base_urlto reados.environ.get(PORTKEY_BASE_URL_ENV)(and then fall back toPORTKEY_PROXY_ENV). - Add regression tests validating env resolution, precedence, and default behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/test_set_base_url.py |
Adds regression tests for set_base_url env handling and precedence. |
portkey_ai/api_resources/utils.py |
Fixes env lookup to use the env-var name constant (PORTKEY_BASE_URL_ENV) rather than the URL value. |
portkey_ai/api_resources/global_constants.py |
Introduces PORTKEY_BASE_URL_ENV constant consistent with existing env-var-name constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,48 @@ | |||
| import pytest | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title: Honor the
PORTKEY_BASE_URLenvironment variable inset_base_urlDescription:
set_base_url(portkey_ai/api_resources/utils.py) passed thePORTKEY_BASE_URLURL value constant (
"https://api.portkey.ai/v1") toos.environ.getinstead ofthe env-var name. It therefore looked up an environment variable literally named
https://api.portkey.ai/v1(alwaysNone), so thePORTKEY_BASE_URLenvironmentvariable was silently never read.
PORTKEY_BASE_URL_ENV = "PORTKEY_BASE_URL"name constant inglobal_constants.py, next to the existingPORTKEY_API_KEY_ENVandPORTKEY_PROXY_ENV, and use it inset_base_url. This matches the codebase'sexisting
_ENV-suffixed env-var-name convention.base_urlargument >PORTKEY_BASE_URLenv >PORTKEY_PROXYenv > api_key-gated default (PORTKEY_BASE_URLvalue when anapi_keyis present, otherwiseLOCAL_BASE_URL).tests/test_set_base_url.py) covering envresolution, precedence, and the defaults.
Motivation:
Before this fix, setting
PORTKEY_BASE_URL(the natural env var, and the one theSDK's own tests and
examples/demo.pyuse) to point at a self-hosted gateway had noeffect: the client fell through to
http://localhost:8787/v1(no api_key) or thepublic cloud URL
https://api.portkey.ai/v1(with an api_key), so requests silentlywent to the wrong host. Only
PORTKEY_PROXYworked as an env override. Reading thecorrectly-named variable makes
PORTKEY_BASE_URLbehave as documented and as thetests/examples already assume.
The other two references to the
PORTKEY_BASE_URLvalue constant are intentionallyleft untouched, because they legitimately use the URL value (not the env-var name):
default_api_key'sbase_url == PORTKEY_BASE_URLcomparison, anddefault_base_url'sos.environ.get(PORTKEY_PROXY_ENV, PORTKEY_BASE_URL)default.Related Issues:
None found (no existing open issue). Confirmed at runtime against
main.Test plan (in the PR / for reviewers)
Red -> green: reverting only the two source files (keeping the new test) makes the 3
env-honoring assertions fail with the exact buggy output (localhost / public-cloud /
PROXY fallthrough); the 4 guard tests pass on both the pre-fix and fixed code, so
they are not tautological.