Skip to content

fix(python): treat blank API keys as unset and read OPENAI_ORGANIZATION - #446

Open
Mohammed Alkindi (MohammedAlkindi) wants to merge 3 commits into
microsoft:mainfrom
MohammedAlkindi:fix/python-model-env-handling
Open

Mohammed Alkindi (MohammedAlkindi) wants to merge 3 commits into
microsoft:mainfrom
MohammedAlkindi:fix/python-model-env-handling

Conversation

@MohammedAlkindi

@MohammedAlkindi Mohammed Alkindi (MohammedAlkindi) commented Oct 7, 2026 •

Copy link
Copy Markdown

The Python create_language_model read the organization header only from OPENAI_ORG, while both example READMEs and typescript/src/model.ts use OPENAI_ORGANIZATION. Setting the documented variable therefore sent no OpenAI-Organization header.

It now reads OPENAI_ORGANIZATION first and falls back to OPENAI_ORG, so existing setups keep working.

Three tests in python/tests/test_create_language_model.py cover the new variable, the fallback, and the precedence when both are set. pytest in python/: 25 passed. pyright 1.1.358 reports no errors in the two changed files.

A blank OPENAI_API_KEY, as left by copying .env.example and filling in only the Azure section, chose OpenAI and sent an empty bearer token. Keys are now chosen by value, as typescript/src/model.ts does.
The Python port read only OPENAI_ORG, while the README and the TypeScript port use OPENAI_ORGANIZATION. OPENAI_ORG is still read as a fallback.

@robgruen robgruen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please see my comment about auth mode. If we don't want to implement that in this PR than the change I would accept is just the fallback for the OPENAI_ORGANIZATION -> OPENAI_ORG variable.

@robgruen

Copy link
Copy Markdown
Contributor

I worry about this change breaking existing TypeChat users that rely on existing behavior. It is possible to use keyless authentication and/or supply credentials after creating the model instance. A cleaner fix would be to add support for proper automatic credential acquisition/refresh. So, if we are going to make a breaking change let's move everyone forward to better auth handling.

As is, this change isn't breaking so we would be changing behavior with no interface change and so it could silently cause issues. We should avoid that if possible.

Reverts the blank-key change from 94e33e3 so a present but empty
OPENAI_API_KEY still selects OpenAI, as it did before. Callers that
supply credentials later or rely on the current selection keep working.
This PR now only adds the OPENAI_ORGANIZATION fallback.
@MohammedAlkindi

Copy link
Copy Markdown
Author

Agreed, the blank-key change could break callers silently. f61c6bf reverts it, so model selection behaves exactly as on main, and the PR is now only the OPENAI_ORGANIZATION read with OPENAI_ORG kept as the fallback. Python tests pass (25) and pyright reports no errors on the changed files.

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.

2 participants