Conversation
a70df45 to
c69d16f
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding a scoped credential override for the bundled OpenAI image provider. The premise remains valid on current main: it still only checks OPENAI_API_KEY at plugins/image_gen/openai/__init__.py:176 and :238, then constructs openai.OpenAI() at :273.
Problems
- The PR is currently
DIRTYagainst main. Current main moved client construction toplugins/image_gen/openai/__init__.py:273, before theimages.edit()/images.generate()split. Salvage should carry the explicit resolved key into that shared constructor so both paths use it. - Add an edit-path assertion: current edits invoke
client.images.edit()atplugins/image_gen/openai/__init__.py:297, while the added tests cover generation only. - Update
website/docs/user-guide/features/image-generation.md:195, which currently names onlyOPENAI_API_KEYfor this backend.
Suggested changes
- Resolve once before the shared current-main client constructor and use
openai.OpenAI(api_key=api_key). - Add a custom
key_envedit test and document the config key plus alias.
Automated hermes-sweeper review.
| @@ -224,7 +251,7 @@ def generate( | |||
| } | |||
There was a problem hiding this comment.
Current main now creates its single OpenAI client before the images.edit() / images.generate() branch at plugins/image_gen/openai/__init__.py:273. When salvaging this dirty PR, apply the explicit api_key to that shared constructor and add an edit-path test so the override covers both operations.
|
Rebased/port onto current
Force-pushed branch |
b493b9f to
810e372
Compare
|
Ready for re-review after port onto current
@teknium1 @kshitijk4poor PTAL when free. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the OpenAI image-provider credential override: both resolve image_gen.openai.key_env (with api_key_env as an alias), fall back to OPENAI_API_KEY, and pass the resolved key to the OpenAI client. #33664 mixes that fix with unrelated base-URL and Docker media-delivery changes, while #37209 isolates the credential change on current main and covers both generation and edit paths.
Related pull requests
- #33664 [closed]
duplicate— (+342/-9) — superseded by #37209: The closed PR implements the same credential-resolution core, but its diff also contains base_url support and unrelated Docker workspace media-path translation, so it remains relevant as the broader predecessor rather than the merge candidate. - #37209
related— (+83/-6) — keep open pending the remaining review item: The focused diff resolves the configured key with OPENAI_API_KEY fallback and passes it through the shared client constructor used by both generate and edit, with tests for both paths. It addresses the contributor's keep_open review request for a custom key_env edit test, but the requested user-guide documentation of the config key and api_key_env alias is still absent from the diff.
Duplicates
#33664 and #37209 substantially duplicate the image_gen.openai key_env credential override; #37209 is the clean, current-main, feature-isolated successor.
Suggested consolidation
Keep #37209 as the consolidation target and merge it after adding the contributor-requested documentation for image_gen.openai.key_env and its api_key_env alias; then #33664 can remain closed as superseded by #37209. This follows the visible keep_open review on #37209 rather than overriding it: the generate/edit implementation concern is resolved in the diff, but the documentation requirement is not.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup33664 ["PRs duplicating each other"]
P33664["PR #33664 (closed)"]
P37209["PR #37209 (open)"]
end
class P33664 closed
class P37209 open
class P37209 target
click P33664 "https://github.com/NousResearch/hermes-agent/pull/33664"
click P37209 "https://github.com/NousResearch/hermes-agent/pull/37209"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 1 kB of issue/PR text, 2 kB of discussion (4 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Allow image_gen.openai.key_env to select a non-default API key env var and pass the resolved key into the shared OpenAI client used by both images.generate() and images.edit().
|
Rebased onto current |
810e372 to
0739191
Compare
Summary
image_gen.openai.key_envsupport for the OpenAI image providerimage_gen.openai.api_key_envas a backward-compatible aliasopenai.OpenAI(api_key=...)OPENAI_API_KEYwhen no override env is configured or setTesting
./.venv/bin/python -m pytest tests/plugins/image_gen/test_openai_provider.py -q