fix: log non-OpenAI format= assumption once at init (#1502) - #1514
fix: log non-OpenAI format= assumption once at init (#1502)#1514r72ntkttwb0f2i wants to merge 5 commits into
Conversation
…ting#1502) Signed-off-by: r72ntkttwb0f2i <r72ntkttwb0f2i@users.noreply.github.com>
…ting#1502) Signed-off-by: r72ntkttwb0f2i <r72ntkttwb0f2i@users.noreply.github.com>
|
Thanks for the contribution @r72ntkttwb0f2i! I've kicked off the acceptance tests and we should be able to merge after they are done. The change seems reasonable. |
|
looks like the linters failed: |
planetf1
left a comment
There was a problem hiding this comment.
Verified against current HEAD, the CI job logs, and issue #1502's own proposed-fix text (not just re-reading the diff). Two real, pre-existing issues below with one-line fixes; the CI failure across all three Python quality jobs is a plain ruff format miss, not a logic problem, also fixed inline.
One finding falls outside this diff's hunks so GitHub won't let me anchor it inline: mellea/backends/openai.py:189 — self._base_url = base_url never falls back to os.getenv("OPENAI_BASE_URL"). If base_url isn't passed but the env var is set, self._base_url stays None, so _server_type() a few lines down defaults to OPENAI and this PR's new init-time log never fires — even though the SDK will actually route to whatever host the env var names. Pre-existing, but worth closing now that this log is being promoted to the authoritative construction-time notice. Suggested fix: self._base_url = base_url or os.getenv("OPENAI_BASE_URL").
One more thing, not tied to a single line: #1493 (open, targeting #1491) rewrites the same block in _generate_from_chat_context_standard — the if _format is not None: if self._server_type == _ServerType.OPENAI: ... else: ... split — as part of unifying schema handling. Whichever of #1493 / #1514 merges second will hit a source conflict there. Worth sequencing deliberately rather than merging blind.
| "JSON schemas passed into `format=`. If you encounter a server-side " | ||
| "error when using format=, then you found an exception to this " | ||
| "assumption. Please open an issue at " | ||
| "github.com/generative_computing/mellea with the stack trace and " |
There was a problem hiding this comment.
Pre-existing typo carried over from the deleted log line: the repo slug elsewhere in the codebase (README, GOVERNANCE, bedrock.py, ollama.py) is always hyphenated. As written this is a dead link.
| "github.com/generative_computing/mellea with the stack trace and " | |
| "github.com/generative-computing/mellea with the stack trace and " |
| if self._base_url is not None | ||
| else _ServerType.OPENAI | ||
| ) # type: ignore | ||
| if self._server_type != _ServerType.OPENAI: |
There was a problem hiding this comment.
Flagging in case another reviewer raises this: moving the log to fire on server type alone, regardless of whether format= is ever used, isn't scope creep — issue #1502's own "Proposed fix, Option 1" asks for exactly this ("tie the message to backend setup rather than the generate loop"). This matches the issue as written.
| OpenAIBackend( | ||
| model_id="gpt-4o", | ||
| api_key="fake-key", | ||
| base_url="http://localhost:9999/v1", | ||
| ) | ||
| OpenAIBackend( | ||
| model_id="gpt-4o", | ||
| api_key="fake-key", | ||
| base_url="http://localhost:9999/v1", | ||
| ) |
There was a problem hiding this comment.
This is the actual cause of the quality (3.11/3.12/3.13) CI failures — ruff format collapses these onto one line each. Not a logic issue; same fix needed at line 466-470 below.
| OpenAIBackend( | |
| model_id="gpt-4o", | |
| api_key="fake-key", | |
| base_url="http://localhost:9999/v1", | |
| ) | |
| OpenAIBackend( | |
| model_id="gpt-4o", | |
| api_key="fake-key", | |
| base_url="http://localhost:9999/v1", | |
| ) | |
| OpenAIBackend( | |
| model_id="gpt-4o", api_key="fake-key", base_url="http://localhost:9999/v1" | |
| ) | |
| OpenAIBackend( | |
| model_id="gpt-4o", api_key="fake-key", base_url="http://localhost:9999/v1" | |
| ) |
| OpenAIBackend( | ||
| model_id="gpt-4o", | ||
| api_key="fake-key", | ||
| base_url="https://api.openai.com/v1", | ||
| ) |
There was a problem hiding this comment.
Same ruff format fix as above.
| OpenAIBackend( | |
| model_id="gpt-4o", | |
| api_key="fake-key", | |
| base_url="https://api.openai.com/v1", | |
| ) | |
| OpenAIBackend( | |
| model_id="gpt-4o", api_key="fake-key", base_url="https://api.openai.com/v1" | |
| ) |
Signed-off-by: r72ntkttwb0f2i <r72ntkttwb0f2i@users.noreply.github.com>
Signed-off-by: r72ntkttwb0f2i <r72ntkttwb0f2i@users.noreply.github.com>
|
Thanks for the review - addressed in the latest commits: uff format on the new unit tests
Re: sequencing with #1493 - happy to rebase if that lands first. |
Signed-off-by: r72ntkttwb0f2i <r72ntkttwb0f2i@users.noreply.github.com>
Summary
format=assumption INFO log from everygenerate()call toOpenAIBackend.__init__, so local / OpenAI-compatible providers do not spam logs on each request (OpenAIBackendlogs a per-call INFO message when using non-OpenAI providers — should log once at init or at DEBUG #1502).Test plan
pytest test/backends/test_openai_unit.py::test_non_openai_format_assumption_logged_once_at_init test/backends/test_openai_unit.py::test_openai_platform_skips_format_assumption_logformat=callFixes #1502