fix(global_config): fail fast when the parent openai version violates nemo-gym's constraint; explicit opt-in for version skew - #2447
Conversation
2645685 to
6f65c8f
Compare
ananthsub
left a comment
There was a problem hiding this comment.
i've added one blocking comment inline on the risk of parent vs child process openAI version differences. previously this would be caught at install time, but now we have a risk of version skew
|
/claude review |
|
SHIP — LGTM, no reliability concerns. Reviewed the openai version-skew gate in Correctness:
The one behavioral change — a new default hard-fail on version skew — replaces a silently broken state (dry-run prefetch baking empty venvs) with fail-fast plus a documented escape hatch. Net operability win. Test coverage is thorough: all three pin/skew/opt-in branches plus the helper's edge cases are exercised. No async, verifier, scorer, reward-aggregation, or public-API surface touched. |
…2476) `wait_for_dry_run_spinup` polled each process until it exited, then dropped the exit code. A dry run builds each server's venv and exits, so a finished process is the expected outcome here rather than the failure `poll()` treats it as. We need to use the exit code to distinguish success from failure. `gym env start` with `dry_run=true` reported the run as complete either way. The code has to be checked because uv creates the venv before installing into it. A failed install still leaves an interpreter and an activate script behind. That is enough to satisfy `should_skip_venv_setup` in `setup_env_command`, so with `skip_venv_if_present` a later run skips setup and reuses the venv. The first visible symptom is then an ImportError from a server, long after the install that caused it, with nothing pointing back at it. This covers any dependency failure in a dry run rather than one particular package: - a typo in a server's `requirements.txt` - a yanked or unavailable release - a network failure part-way through resolution - a dependency conflict between a server and `nemo-gym` All of these reported success before. `gym env prefetch` already checks the return code of each setup process and raises `SystemExit`. This brings the dry-run path in line with it. The failure in #2447 is the last case in that list: a parent openai version that `nemo-gym`'s own constraint rejects makes every sub-venv resolution unsatisfiable, and this is the reason it surfaced as empty venvs rather than as a build error. That PR is still worth having on its own terms, since a resolvable install beats a clear error about an unresolvable one. This is about the silence, not the conflict. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com> Co-authored-by: Marta Stepniewska-Dziubinska <marta-sd@users.noreply.github.com>
5d60b77 to
71b5398
Compare
… when nemo-gym's own constraint accepts it head_server_deps pins the parent process's openai version into every sub-venv for consistency. When the parent environment ships an openai release outside nemo-gym's own requirement (e.g. openai 2.52.x preinstalled in the base image while nemo-gym caps openai<=2.7.2), that pin makes every sub-venv resolution unsatisfiable. uv/pip then fail the resolve, and the dry-run venv prefetch silently bakes venvs that contain nothing but pip - the failure only surfaces much later at server startup as missing imports. Fix: before appending 'openai==<parent version>' to head_server_deps, check the parent's version against nemo-gym's own openai requirement (via importlib.metadata). If it does not satisfy the constraint, omit the pin and let each sub-venv resolve openai from nemo-gym's requirement instead (nemo-gym is installed into the sub-venvs, so its constraint still applies there). Behavior is unchanged whenever the parent's openai is compatible, and the check conservatively falls back to the original pin-the-parent behavior when the constraint cannot be determined (packaging missing, marker'd or absent openai requirement, metadata lookup failure). Unit tests cover the compatible/incompatible/fallback paths. Signed-off-by: Michal Futrega <mfutrega@nvidia.com>
Silently dropping the pin would leave no breadcrumb for anyone debugging a parent/sub-venv openai version mismatch. Print which version was not pinned and why, mirroring the parser's other configuration notices. Signed-off-by: Michal Futrega <michal.futrega@gmail.com>
…sion skew Address review: instead of silently omitting the openai pin from head_server_deps, raise ConfigError at parse time when the parent process's openai violates nemo-gym's own constraint, with a message naming both versions and the remedy. A new reserved top-level key allow_openai_version_skew=true opts into the previous fallback (server venvs resolve openai from nemo-gym's constraint) with a warning log. Canonicalize the requirement name per PEP 503. Add parser-level tests for all three modes (compatible pin, incompatible raise, opt-in skew). Signed-off-by: Michal Futrega <michal.futrega@gmail.com>
…lean for skew opt-in, message-content assertions - Drop concrete openai versions from docstrings (they drift and trip grep-based tooling) - Validate allow_openai_version_skew as a boolean: a string like "false" is truthy and must not silently enable skew; raise ConfigError instead - Assert the fail-fast error and the opt-in warning both name the parent version and nemo-gym's requirement; cover the non-boolean rejection Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Michal Futrega <michal.futrega@gmail.com>
960dc83 to
20a41a4
Compare
Problem
head_server_depspins the parent process's openai version into every server venv. When the parent environment ships an openai release outside nemo-gym's own constraint (e.g. openai 2.52.x preinstalled in a base image while nemo-gym capsopenai<=2.7.2), that pin makes every server venv resolution unsatisfiable. This does not fail cleanly: the dry-run prefetch bakes venvs that contain nothing but pip, and the problem surfaces only at runtime as import errors far from the cause.Change (after review)
ConfigErrorat parse time when the parent openai violates nemo-gym's constraint, naming both versions and the remedy. This preserves the install-time guarantee the previous behavior intended to give, minus the empty-venv mystery.allow_openai_version_skew: trueopts into letting server venvs resolve openai from nemo-gym's own constraint, with alogging.warningnaming both versions. Parent and servers exchange requests across the HTTP/JSON boundary, so the skew surface is the JSON contract, not Python type identity — we run this topology in production (parent 2.52.x, servers on the nemo-gym pin) on 64-node RL trainings.Tests
Parser-level (through
get_global_config_dict): compatible parent → pin present inhead_server_deps; incompatible parent →ConfigErrornaming the opt-in key; incompatible parent + opt-in → pin omitted. Plus unit coverage of the constraint matcher (case-insensitive names, marker'd requirements skipped, missing metadata → conservative pin-preserving fallback).