fix(config): fail fast with a clean error on a malformed head_server block - #3561
Open
sven-rankov wants to merge 2 commits into
Open
sven-rankov wants to merge 2 commits into
sven-rankov wants to merge 2 commits into
Conversation
sven-rankov
marked this pull request as ready for review
September 20, 2026 19:36
Contributor
Author
|
Friendly ping on this one. @yaoyu-33 Would you be able review? |
…block A wrong-typed `head_server` value (typically a `++head_server.port=<not an int>` override) was only validated when `ServerClient.load_head_server_config` built a `BaseServerConfig`, deep inside the command. `BaseServerConfig` is not a CLI config, so the router re-raised the pydantic error: `gym eval run` ended in a raw traceback, `gym env start` initialised Ray before failing, and `gym env validate` / `gym env resolve` accepted the value and exited 0. Validate the block against `BaseServerConfig` in `GlobalConfigDictParser.parse()` right after its defaults are filled, raising the new `HeadServerConfigMalformedError(ConfigError, ValueError)` with an actionable message, the same way the parser handles `config_paths`. Every affected command loads the config first and is wrapped in `exit_cleanly_on_config_error`, so all of them now print one `Error:` line and exit 1 before any server starts. The router's re-raise for non-CLI models is intentionally unchanged (`test_non_config_validation_error_is_reraised`); the dropped-host half of the issue was already fixed by NVIDIA-NeMo#3188. Closes NVIDIA-NeMo#2686 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Tsvetan (Sven) Rankov <tsvetan.rankov@gmail.com>
The first commit validated `head_server` about twenty lines after the parser had already read `head_server.port` for the port allocator, so `++head_server=<scalar>` still died with an AttributeError at that earlier `.get`, and a quoted `port: "11000"` bypassed the collision guard (the string sat in `initial_disallowed_ports`, never matching an allocated int). Merge the two blocks into one at first read: reject a non-mapping block with the same ConfigError, fill the defaults, validate against BaseServerConfig, and hand the validated port to the allocator. The stored block is still left as written, so `gym env resolve` output is unchanged. The hint now also names `num_workers`, the third BaseServerConfig field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Tsvetan (Sven) Rankov <tsvetan.rankov@gmail.com>
sven-rankov
force-pushed
the
sven-rankov/fix-2686-malformed-head-server-config
branch
from
September 23, 2026 14:50
46ca5cf to
6544cd3
Compare
This branch has not been deployed
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
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.
What does this PR do?
Closes #2686.
A
head_servervalue with the wrong type — typically a++head_server.port=<not an int>override — was only validated whenServerClient.load_head_server_configbuilt aBaseServerConfig, deep inside the command.BaseServerConfigis not aBaseNeMoGymCLIConfig, so_handle_pydantic_validation_errorre-raised the pydantic error and the user got a 71-line traceback (gym eval run --no-serve),gym env startinitialised Ray before failing, andgym env validate/gym env resolveaccepted the value and exited 0.This PR moves the parser's
head_serverhandling to where the block is first read inGlobalConfigDictParser.parse()(previously it readportfor the port allocator there and only filled the defaults ~20 lines later): fill the defaults, validate the block againstBaseServerConfig, hand the validatedportto the allocator, and raise a newHeadServerConfigMalformedError(ConfigError, ValueError)with an actionable message — the same pattern the parser already uses forconfig_paths(MalformedConfigPathsError). Every affected command loads the config first and is wrapped inexit_cleanly_on_config_error, so all of them now print oneError:line and exit 1 before any server (or Ray) starts.Before / after
gym env validate ++head_server.port=notanint— before (main): exit 0gym eval run --no-serve --agent simple_agent -i in.jsonl -o out ++head_server.port=notanint— before: exit 1, 71 lines on stderr (abridged):After — the same for every command that loads the config (
env validate,env resolve,env start,eval run): exit 1, nothing on stderrNotes:
host) was already fixed by feat(orchestration): return and persist a record of every submission #3188; this PR covers the remaining traceback half._handle_pydantic_validation_error's re-raise for non-CLI models is intentionally left as is:test_non_config_validation_error_is_reraiseddocuments that aValidationErrorfrom a non-CLI model may be a real bug that must propagate. Handling the user-facing block at the source keeps that contract.gym env resolveoutput is unchanged for valid configs. The validated (coerced)portis what now reaches the port allocator, so a quotedport: "11000"is kept off it (before, the string'11000'never matched an allocated int).++head_server=foo) used to fail with anAttributeErrorat the parser's first.get; it now gets the sameConfigErrortreatment with its own message.GlobalAIOHTTPAsyncClientConfig,TelemetryConfig, exporter configs).Validation
Before (main dbd26a9) / after, from the repo root:
gym eval run --no-serve --agent simple_agent -i in.jsonl -o out ++head_server.port=notanint— before: exit 1, 71-line traceback ending inValidationError ... for BaseServerConfig; after: exit 1, the 5-lineError:message above on stdout, nothing on stderr.gym env validate ++head_server.port=notanint— before:✓ Config is valid., exit 0; after:Error: ..., exit 1.gym env resolve ++head_server.port=notanint— before: dumpsport: notanint, exit 0; after:Error: ..., exit 1.gym env validate ++head_server.port=12345— still✓ Config is valid., exit 0.gym env validate ++head_server.host=null— after:Error:naminghead_server.host, exit 1.gym env validate ++head_server=foo— before: exit 1, 29-line traceback ending inAttributeError: 'str' object has no attribute 'get'; after:Error: 'head_server' must be a mapping ofhostandport. Got: 'foo'., exit 1.gym env validate ++head_server.num_workers=abc— after:Error:naminghead_server.num_workers, exit 1.Tests:
TestConfigLoadErrors::test_parse_malformed_head_server_raises_actionable_error(parser level, parametrized over a non-integerport, anullhost, and both at once; asserts every field path, the override hint, theValueErrorcompat contract, and that__cause__is the pydantic error).TestConfigLoadErrors::test_parse_scalar_head_server_raises_actionable_error(head_server: fooraises theConfigErrorwith the override hint instead of anAttributeError).TestConfigLoadErrors::test_parse_keeps_a_head_server_value_pydantic_can_coerce(a quotedport: "8080"still parses and is left as written — the check is lax and validate-only — whiledisallowed_portsholds the validated int8080).TestFriendlyValidationError::test_malformed_head_server_override_is_a_config_error_not_a_traceback(through the realmain(), parametrized overenv validate,env resolve,eval run --no-serve; asserts exit 1,Error:on stdout, no "Config is valid", no traceback).TestFriendlyValidationError::test_malformed_head_server_is_rejected_before_any_server_starts(gym env startthrough the realmain()withRunHelperreplaced by a sentinel that fails if constructed — proves the error fires before Ray or the head server come up).mainfor the issue's reasons (exit 0 / rawValidationError/ the sentinel firing).pytest tests/unit_tests/test_global_config.py tests/unit_tests/test_cli_main.py tests/unit_tests/test_cli.py tests/unit_tests/test_server_utils.py tests/unit_tests/test_server_status.py tests/unit_tests/test_cli_eval.py tests/unit_tests/test_cli_eval_submit.py— 594 passed.pytest tests/unit_tests/(macOS) — 5127 passed; the 53 failures are all pre-existing onmainand unrelated (test_super_vllm_launcher.pybash launcher subtests,test_ci_environment.py,test_e2b_provider.py,test_enroot_provider.py).ruff check/ruff format --check,pre-commit run --all-files— clean.python3 tests/unit_tests/test_fern_docs_links.py— OK.Rollouts
N/A — config parsing / CLI error handling only; no environment, agent, model, or verifier behavior changes.
Compatibility and benchmark impact
head_serverblock that was previously accepted at parse time despite a wrong-typed value (non-integerport,nullhost/port) now fails with aConfigErrorfor every command, instead of a traceback (or exit 0) later. Well-typed configs, including a string port that pydantic coerces (e.g.port: "8080"), are unaffected.gym env resolveoutput for valid configs.HeadServerConfigMalformedErrorsubclassesValueErrorlike the rest of theConfigErrorfamily.Proposed labels (I cannot apply labels): bug, area:config, complexity:low
Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).