hermes_agent: repin to upstream hermes and fix the wrapper for its API - #2052
hermes_agent: repin to upstream hermes and fix the wrapper for its API#2052j-nolan wants to merge 20 commits into
Conversation
ea6a874 to
ceaa05a
Compare
|
the pin is there to support training, i think it should stay until we implement support for blackbox agent training. right now, training will break if you remove the pin |
@cmunley1 got it, thanks for the follow up. Do you have a rough idea of when the blackbox agent training will land? Currently, we are working on a fork of gym just to work around this, it's OK for now but hoping we can converge again one day |
a8d243c to
fa768a2
Compare
|
cherry picking would be fine, it is primarly these changes plus i had to relax an openai version pin in the past which probably has changed too. https://github.com/NousResearch/hermes-agent/tree/nemo-gym-changes |
|
yeah, probably a few weeks for training as some other work is blocking |
|
Thanks @Glorf and @cmunley1 ! I had a quick look at upstreaming the change to hermes. It seems a bit more involved than I hoped: hermes changed quite a bit since the original pin, especially |
6a7e7ac to
965b778
Compare
|
/claude review |
|
/ok to test 94c33a2 |
|
/ok to test 862918c |
| agent._disable_streaming = True | ||
| _original_build_api_kwargs = agent._build_api_kwargs | ||
|
|
||
| def _patched_build_api_kwargs(api_messages): |
There was a problem hiding this comment.
Hermes v2026.8.31 may call this method with tools_for_api=..., but the wrapper only accepts api_messages. This will raise TypeError: unexpected keyword argument 'tools_for_api' when that path is used. Should we preserve the upstream signature and forward tools_for_api to _original_build_api_kwargs?
There was a problem hiding this comment.
Makes sense, thank you for finding it.
hermes-agent's conversation loop calls agent._build_api_kwargs(api_messages, tools_for_api=tools_for_api) whenever a call uses a scoped toolset (not the agent's full default set) — a live, common path, not an edge case. Since the patch replaces the method entirely with a function that only accepted api_messages, that call would raise: TypeError: unexpected keyword argument 'tools_for_api'. Confirmed both ways empirically: the old wrapper fails with exactly that error, the fixed one forwards the kwarg cleanly. Signed-off-by: Riccardo Roveri <rroveri@nvidia.com>
hermes-agent's own setup.py refuses to build as a wheel unless HERMES_NIX_BUILD=1 is set (it expects installation via its shell installer, Docker image, or Nix, not a plain pip/uv build). Scoped to exactly one server: dir_path.name == 'hermes_agent' is false for every other server, so this is a no-op — literally the same generated command as before — everywhere else. No cross-contamination between sibling server builds either, since each runs as its own subprocess with its own env dict copied fresh from the parent process. Verified live: ran the actual generated install command via run_command() against a fresh clone, no manual env var set — hermes-agent installed and imported successfully (previously failed with 'Building wheels or sdists for hermes-agent is not supported'). Signed-off-by: Riccardo Roveri <rroveri@nvidia.com>
|
@cmunley1 Also fixed the Unit tests / Server suite CI failure (shard 4) — hermes-agent's build refuses to produce a wheel without HERMES_NIX_BUILD=1, so I added it to hermes_agent's own venv setup, scoped to just that server. |
pyproject.toml intentionally pins openai==2.44.0 twice (once in [project].dependencies, once in override-dependencies, to force uv's resolver past hermes-agent's own openai==2.24.0 hard-pin). The grep that extracts OPENAI_PIN matched both occurrences, producing a two-line string that broke `pip install` with an "Invalid requirement" error and failed the prewarm-harness SLURM job (verified against gitlab-master.nvidia.com/interactive-agents/slurm-evaluations jobs 424346546 and 424441611). Pipe the extraction through `sort -u` so the duplicate matches collapse to the single distinct pin. Signed-off-by: Pascal Bérard <pberard@nvidia.com>
The model server forwards to engines that accept fields OpenAI does not define, and callers send them: vLLM's chat_template_kwargs drives a model's thinking mode, and clients label tool results with the tool's name. Forbidding extras rejects the request outright, so the caller loses the field and the conversation with it. Requests are inbound from callers Gym does not control, so carrying what is not modelled is the right default for a proxy; being strict about what Gym itself sends is unaffected. Message contents are still validated, so a malformed role or a broken tool call is still rejected. The strictness arrived with the openai pin in NVIDIA-NeMo#2456, whose issue asked only to support newer vllm versions. It was not the point of that change, and its effect is to reject the extension field vllm itself needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NVIDIA-NeMo#3061 added guards that the tool-call name normalization must not loosen the request model. Relaxing the schema loosens it deliberately, so those assertions now describe behaviour the model no longer has. Invert the two that assert rejection, and add one that pins down what did not change: a tool call missing its required function is still rejected, so carrying unknown keys has not turned off structural validation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| # Inbound requests carry vendor extensions this schema does not enumerate, and the engines | ||
| # they are forwarded to understand them. Rejecting the request loses the caller's field and | ||
| # the whole conversation with it; forwarding what we do not model is the proxy's job. | ||
| model_config = ConfigDict(extra="allow") |
There was a problem hiding this comment.
do we need this? I think we did forbid on purpose cc @bxyu-nvidia
| # Absent from the OpenAI type but sent by real clients and accepted by real servers. | ||
| # Without it the request schema rejects the whole conversation, so an agent that calls a | ||
| # tool cannot receive the result. | ||
| name: NotRequired[str] |
| env_setup_cmd = ( | ||
| f"{uv_venv_cmd}{prefix_cmd} && source {venv_activate_fpath} && " | ||
| f"{extra_env_exports}{install_cmd}{prefix_cmd}" | ||
| ) |
There was a problem hiding this comment.
couldnt we just set this envvar in app.py setup_webserver or something. would like to avoid core change when possible
There was a problem hiding this comment.
on further thought i see how this might need a core change if
HERMES_NIX_BUILD=1 gym env start --environment hermes_math ...
is not sufficient... hmmm.. cc @bxyu-nvidia @ananthsub
Repins gym's hermes_agent from the cmunley1 fork to upstream NousResearch/hermes-agent (569b912, today's tip), drops the fork, and updates the wrapper for that build's AIAgent API. Raises gym's openai cap so the pin co-resolves.
Why
The current pin is
cmunley1/hermes-agent@26bb847a, which is upstreambe43bee1plus a single one-line openai relax. Pointing at upstream directly removes the fork as a maintenance point. Upstream's AIAgent API has changed since April, so the wrapper needs updating to match.Changes
hermes_agent/requirements.txt: pinNousResearch/hermes-agent@569b912(wascmunley1/hermes agent@26bb847a).pyproject.toml: openai cap raised<=2.7.2to<=2.24.0, because upstream hermes pinsopenai==2.24.0. This is a gym-wide ceiling change; it was smoke-tested against the gym unit suite (1145 passed; the only failures are the unrelated opensandbox tests from a missing optional dep).hermes_agent/app.py, updated for the 569b912 AIAgent API:use_streamingandpersist_sessionconstructor kwargs were removed upstream, so dropped (non-streaming and no persistence are now the default paths).insert_reasoning=Trueis nowreasoning_config={"enabled": True}.temperatureis nowrequest_overrides={"temperature": ...}.stream=truewith HTTP 422). Fixed by settingagent._disable_streaming=True(the successor touse_streaming=False) plus forcingstream=Falsein the existing_build_api_kwargspatch.Validation
The signature smoke test passes, but it only catches signature-level breaks. The streaming and grading issues above were only surfaced by a real run, so the acceptance gate is an actual Terminal Bench pass, not the unit test. Validated end to end at 62/89 resolved on Terminal Bench 2.1 with hermes 569b912 driving Claude Opus (verifier working, no constructor or stream errors).