fix(ci): use ++ for token_capture override in SC functional tests - #4031
Closed
terrykong wants to merge 1 commit into
Closed
fix(ci): use ++ for token_capture override in SC functional tests#4031terrykong wants to merge 1 commit into
terrykong wants to merge 1 commit into
Conversation
The SingleController L1 functional test passes token_capture.enabled=true
as a plain Hydra override, but examples/nemo_gym/grpo_qwen3_30ba3b_instruct.yaml
does not declare a token_capture block. Hydra composes in struct mode, so the
run dies before training starts:
omegaconf.errors.ConfigKeyError: Key 'token_capture' is not in struct
hydra.errors.ConfigCompositionException: Could not override 'token_capture.enabled'.
Use the ++ (add-or-override) prefix, matching how the same script already
passes the other SingleController-only knobs to this shared Gym config
(++data_plane.*, ++async_rl.*). The downstream
if [[ "$*" == *token_capture.enabled=true* ]]
check in grpo_async_gym_single_controller.sh still matches, so the
finalize/total_ms assertion continues to run.
Signed-off-by: Terry Kong <terryk@nvidia.com>
Collaborator
Author
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 ?
Unblocks
L1_Functional_Tests_SingleController(and its GB200 twin) on #4022.Both jobs fail before training starts:
L1_Functional_Tests_SingleController.shpassestoken_capture.enabled=trueas a plain Hydraoverride, but the config that test runs —
examples/nemo_gym/grpo_qwen3_30ba3b_instruct.yaml—has no
token_captureblock. Hydra composes in struct mode, so overriding a key that is not inthe YAML is an error.
MasterConfig.token_capturehaving a Pydantic default does not help,because that default is applied after Hydra composition.
The fix is the
++(add-or-override) prefix, which is how this same script already passes theother SingleController-only knobs into this shared Gym config —
++data_plane.*,++async_rl.*. One line.Details
This is not a #4022 regression. It reproduces on
main:main'sL1_Functional_Tests_SingleController.shhas the same un-prefixed override.main'sgrpo_qwen3_30ba3b_instruct.yamlhas notoken_capturekey.It came in with #3837 (
7036e5d16), which added the test line and thetoken_capturefeature butdid not add the key to the Gym config the test runs. Targeting #4022's branch because that is what
the failure is currently blocking; happy to retarget at
maininstead if you would rather it landindependently.
Why not add
token_capture:to the YAML instead? That config is shared by eight otherfunctional scripts, most of them not SingleController, so declaring an SC-only block there has a
wider blast radius than the caller-side
++. Either fix works — say the word if you prefer theconfig-side one and I will switch it.
The test still asserts what it is meant to.
grpo_async_gym_single_controller.shgates itsextra check on the argument string:
++token_capture.enabled=truestill matches that glob, so the finalizer assertion keeps running —the test is not being quietly turned into a no-op.
Verification
Reproduced and fixed locally, CPU only, against the real config loader:
(driving
nemo_rl.utils.config.load_config+parse_hydra_overridesonexamples/nemo_gym/grpo_qwen3_30ba3b_instruct.yaml, which is the exact path that raises in CI)The glob check above was verified separately in bash. The GPU run itself is not something I can
execute — that needs CI.
Issues
Unblocks CI on #4022.