Skip to content

fix: apply config-file overrides before argument validation - #1914

Open
Shi-Dong wants to merge 4 commits into
mainfrom
shi/config-overrides-before-validation
Open

fix: apply config-file overrides before argument validation#1914
Shi-Dong wants to merge 4 commits into
mainfrom
shi/config-overrides-before-validation

Conversation

@Shi-Dong

@Shi-Dong Shi-Dong commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

--custom-config-path lets a YAML file override any argument, but the override loop ran near the end of miles_validate_args. Every validation and derivation earlier in the function therefore used the pre-override values:

  • use_critic is derived from advantage_estimator before the override, so selecting the advantage estimator via the config file leaves use_critic stale: the critic worker group and its placement are sized for the wrong estimator, and the async off-policy guard introduced in fix: require explicit off-policy correction for async PPO training #1829 (which keys on use_critic) silently never fires.
  • In colocate mode, rollout_num_gpus only reserves critic GPUs when use_critic is already set, so a config-file estimator switch under-provisions the rollout GPUs.
  • offload is expanded into offload_train / offload_rollout and then deleted before the override ran, so offload: true in a config file was a silent no-op.
  • Validation asserts above the override point (e.g. estimator-specific requirements) never saw the overridden values.

This PR extracts the override loop into _apply_custom_config_overrides and calls it as the first statement of miles_validate_args. An argument overridden via the config file now behaves exactly as if it had been passed on the command line: it participates in every subsequent validation and derivation. The --custom-config-path help text documents this.

One intentional behavior change: values that validation derives (such as use_critic) can no longer be forced through the config file, because the derivation now runs after the override — set the source argument (e.g. advantage_estimator) instead.

Stacked on #1829.

Test plan

  • New tests in tests/fast/utils/test_arguments.py (TestApplyCustomConfigOverrides):
    • no custom_config_path is a no-op
    • a namespace without the custom_config_path attribute is a no-op (tests invoke miles_validate_args with partial namespaces)
    • empty YAML file is a no-op
    • existing keys are overridden and unknown keys are added
  • python -m py_compile on both touched files
  • black --check, isort --check-only, and ruff check clean on both touched files

Note: tests/fast/utils/test_arguments.py imports miles.utils.arguments, which requires sglang, so the new tests were verified on this branch by exercising the extracted function body directly (same cases as committed), plus an AST check that the override call is the first statement of miles_validate_args and the old inline block is gone; CI runs the real test file.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Base automatically changed from shi/async-ppo-policy-contract to main July 29, 2026 01:26
Shi-Dong added 2 commits July 28, 2026 21:36
…before-validation

# Conflicts:
#	miles/utils/arguments.py
#	tests/fast/utils/test_arguments.py
Tests call miles_validate_args with partial namespaces; now that the override helper runs first, it must not require the attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant