docs: notice recommending NeMo-Gym for benchmarks and rollouts; pin mcp<2.0 - #1512
Merged
Conversation
…outs Add a short notice at the top of the README (and the mirrored docs landing page) recommending NeMo-Gym for evaluation and for contributing new benchmarks, while making clear that Nemo-Skills evaluation continues to work and is still maintained. The notice also points at the existing path for running NeMo-Gym benchmarks through the Nemo-Skills pipeline machinery. `ns nemo_gym_rollouts` has been available for a while but had no documentation, so add a pipelines page covering self-hosted and pre-hosted policy servers, the Hydra passthrough arguments, seed-based fan-out, and container resolution. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Make the notice clearer that we encourage moving rollout workflows to NeMo-Gym and implementing new benchmarks there, while Nemo-Skills continues to gain orchestration and execution support for running those benchmarks. Drop the ns nemo_gym_rollouts documentation page. The interface is still experimental and likely to change, so it is mentioned in the notice rather than documented in full. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughREADME and documentation homepage notes announce the NeMo-Gym migration and experimental rollout interface. Dependency requirements constrain MCP below 2.0, update pipeline CLI compatibility, set a minimum W&B version, and revise an HTTPX override comment. ChangesNeMo-Gym Documentation and Dependency Compatibility
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Kipok
approved these changes
Jul 28, 2026
…ery CI run mcp 2.0.0 was published on 2026-07-28 and dropped the deprecated streamablehttp_client alias, keeping only streamable_http_client. The requirement was unpinned, so CI began installing 2.0.0 and failing at import time in nemo_skills/mcp/clients.py. That module is on the import path for the ns CLI itself (cli -> eval -> generate -> model -> mcp.utils -> mcp.clients), so the failure takes down the entire unit test suite at collection, not just the MCP tests. Pin below 2.0 to unblock CI. The forward fix is to rename the three call sites to streamable_http_client and verify the 2.0 transport signature, which is left for a follow-up. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
gwarmstrong
changed the title
gwarmstrong
enabled auto-merge (squash)
July 28, 2026 19:57
Cherry-picked from #1507 (fix/security-dependency-floors), which is the last configuration in which the full CPU suite passed (725 passed, 0 failed on 2026-07-14). With click pinned below 8.2 the ns CLI stops accepting underscore-style option names, so every test that shells out to `ns eval --output_dir=...` or `ns summarize_results --max_seq_len=...` fails with "Missing option '--output-dir'" / "No such option: --max_seq_len". The typer floor moves to 0.16 alongside it because that is the first release compatible with click 8.2 (fixes the make_metavar break that motivated the original pin). Only the CLI-relevant pair is taken here. #1507's litellm, wandb and stem security floors are left to that PR. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Dropping the click<8.2 cap alone was not enough: wandb 0.26.1 only requires click>=8.0.1, so uv still settled on click 8.1.8 and the CPU suite failed identically (10 failed, 685 passed). wandb 0.27.1 is the first release requiring click>=8.2.0, which is what actually moves the resolver. Taken from #1507, the last configuration in which the full suite passed. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
This is the actual root cause of the CPU suite failures, and the reason
removing the repo's own `click < 8.2.0` cap changed nothing: litellm
1.83.14 declares an exact `click==8.1.8` dependency, capping the entire
tree below click 8.2 regardless of what this repo asks for.
uv spelled it out when wandb>=0.27.1 was added:
Because wandb>=0.27.1 depends on click>=8.2.0 and litellm==1.83.14
depends on click==8.1.8, we can conclude that litellm==1.83.14 and
wandb>=0.27.1 are incompatible.
litellm 1.84.10 relaxes that to click>=8.0.0,<9.0 (and clears
GHSA-4xpc-pv4p-pm3w). With it, `uv pip install -e .[dev]` resolves to
click 8.4.2 / typer 0.27.0 / wandb 0.28.1.
Taken from #1507 together with the wandb floor and the click cap removal;
the three only work as a set.
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Root cause of the 10 CPU-test failures. typer 0.27.0 (released
2026-07-15) generates dash-separated option names from the Python
parameter name, so `output_dir` became `--output-dir`. Every `ns`
command, every test and every doc example passes the underscore form,
so the whole suite broke the day after 0.27.0 shipped. `typer` had no
upper bound, so CI picked it up silently.
Verified against a real install of this package rather than a synthetic
typer app, which does not reproduce the difference:
typer 0.26.8 -> ns eval --help shows --output_dir, parses OK
typer 0.27.0 -> ns eval --help shows --output-dir, rejects it
The cap is deliberately narrow. Lifting it needs a repo-wide rename of
the option style in commands, tests and docs, which belongs in its own
change.
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
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
2 participants
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.
Adds a short notice to the top of the README and the docs landing page encouraging users to move their rollout workflows to NeMo-Gym and to implement new benchmarks there, while noting that Nemo-Skills will continue to add support for running them. Also pins
mcp<2.0, since mcp 2.0.0 (released 2026-07-28) removed thestreamablehttp_clientalias thatnemo_skills/mcp/clients.pyimports, which was breaking CI at import time for every run.Summary by CodeRabbit
ns nemo_gym_rolloutscommand and warned its interface may change.mcp<2.0, raisedwandbto>=0.27.1, and modernized pipeline constraints by removing the strict Click pin and addingtyper >= 0.16.httpx[http2]override (no version changes).