ci(config): fail the schema check on real drift, not just report it - #963
Draft
WangLingxun wants to merge 5 commits into
Draft
ci(config): fail the schema check on real drift, not just report it#963WangLingxun wants to merge 5 commits into
WangLingxun wants to merge 5 commits into
Conversation
WangLingxun
force-pushed
the
ci/fail-on-config-schema-drift
branch
from
August 10, 2026 09:47
c07e100 to
1354598
Compare
`--warn-only` bought a burn-in period while the check found real, unfixed drift without blocking unrelated work. That period should end with the Megatron cleanup: otherwise the findings only reach `$GITHUB_STEP_SUMMARY`, so a reviewer looking at a green `code-lint` has no reason to know the job silently accepted unknown keys. This commit is expected to fail while unknown-key drift remains on `main`. That red run is the gate firing on real drift rather than a fabricated test; the commit must land with or after the cleanup that reduces the report to zero unknown keys. Model-scoped findings stay non-blocking. Such a key is declared by some other model's config class but set on a model that does not read it. Fixing one can move training numerics, so it needs the model owner and a convergence run, not a gate that blocks unrelated PRs. Verified both exit paths locally: a fabricated unknown key under `--path` exits 1 normally and 0 with `--warn-only`; a tree with no unknown keys but model-scoped findings exits 0.
WangLingxun
force-pushed
the
ci/fail-on-config-schema-drift
branch
from
September 14, 2026 09:10
6ed15c0 to
b72dcec
Compare
![pr-pundit-code-review[bot]](https://avatars.githubusercontent.com/u/16860860?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: AMD-AGI/Primus
Summary
No violations found, 3 files reviewed.
Rules checked: 3 (3 passed, 0 failed).
✅ Passed
All 3 rules satisfied. No architecture concerns found.
Posted by PR Pundit — AI-powered code review grounded in this repo's merge history.
There was a problem hiding this comment.
PR Pundit — Test Coverage ✅
Verdict: Covered | Categories: config, other
Test evidence found in this PR:
- CI config change (.github/workflows/ci.yaml: removed --warn-only from schema check step)
- existing pytest test renamed and updated (test_cli_reports_a_misplaced_key_without_gating) asserting cli.main() == 0
- assertions on CLI stdout content (capsys)
✅ 9 requirement(s) satisfied
- Config/CI-only change: PR modifies .github/workflows/ci.yaml (drops --warn-only) and the schema-check exclusion logic, with no kernel, model, or runtime logic touched — so kernel_optimization benchmark/before-after evidence is not applicable.
- Unit test coverage for the changed behavior exists: tests/unit_tests/configs/test_config_schema_check.py renamed to test_cli_reports_a_misplaced_key_without_gating and updated to assert cli.main() == 0 for model-scoped misplaced keys.
- Observable output verification: assertions on CLI stdout via capsys confirm the misplaced-key finding is still reported (warn, not fail).
- Docstring/test updates accompany the behavioral change, keeping documented intent aligned with the new hard-gate semantics.
- MoE token-dispatcher flag-combination requirement (validate_args_on_rocm, use_turbo_permute_padding, moe_use_legacy_grouped_gemm vs use_turbo_grouped_gemm) — not applicable: this PR touches no MoE dispatcher tests or code.
- MLflow artifact/trace error-handling side_effect exception-type requirement — not applicable: no mlflow_artifacts tests or code in this diff.
- Unique
tagfor run_script in trainer tests — not applicable: no trainer/run_script tests added or modified. - qkv_format 'sbhd'/'bshd' layout tests for Primus-Turbo Megatron extensions — not applicable: no qkv_format-dependent logic changed.
- Correct @patch targets for primus.backends.megatron.training.mlflow_artifacts.log_rank_0/warning_rank_0 — not applicable: no patched mlflow helpers in this diff.
Advisory
🔵 A negative-path test asserting that a non-model-scoped schema drift finding (e.g., unknown/renamed top-level key) still causes cli.main() to return a non-zero exit code now that --warn-only has been removed and the check is a hard gate.
The diff adds/renames a test proving the non-gating case (model-scoped misplaced key -> exit 0), but the behavioral change introduced by this PR is that other drift findings now fail CI. Without a complementary failing-exit-code assertion, the hard-gate behavior itself is unverified by tests; it is only exercised implicitly by the CI workflow step.
Rule: bug_fix/new_feature evidence: Unit tests covering the new/changed code paths
Source: repo evidence requirement
PR Pundit test coverage — grounded in this repo's merged PR history.
The shared trainer/model presets and examples carry keys that parse cleanly but have no current reader in either Megatron or Primus. Removing them does not override a backend default or change runtime behaviour. The trainer group includes upstream-deleted RETRO, dynamic-batching and RL arguments plus Primus switches whose consumers disappeared. The model/example group includes removed parallelism and profiler arguments, obsolete hybrid fields, and Primus switches that no patch reads. Several names also occur in Python without consuming the YAML field: `final_logit_softcapping` is only a `fused_softcap()` parameter whose caller uses `router_logit_softcapping`; PyTorch's `record_shapes` keyword is driven by `torch_profiler_record_shapes`; `fused_padded_mla_attention` survives only in a docstring; and standalone hybrid tools hardcode rather than read `mamba_d_conv` and `legacy_tokenizer` from Primus configs. `turbo_fused_grouped_gemm` is removed too. Its own comments call it a future placeholder whose Primus wiring has not landed, so it should be introduced with its consumer instead of silently accepted in advance. Non-default dead settings keep comments where silently deleting the intent would be misleading: Grok2's final-logit soft cap, Qwen3.5's old layernorm 1p switch, and GPT-OSS's removed Turbo padding workaround. The stale `fully_parallel_save` spelling is deleted rather than renamed because its SFT base already sets `ckpt_fully_parallel_save: true`. The unused `diffusion/encoders.yaml` also goes: no config extends it, the encoder code uses equivalent dataclass defaults, and its documentation now points there.
…s in `overrides:` `overrides:` feeds Megatron's argument parser. Nothing in Primus lifts an ALL_CAPS override into `os.environ` -- `schema_check.py` even calls this out as the canonical mistake -- so the nine shell-style keys in `deepseek_v2_lite-BF16-sft-packed.yaml` set nothing at all. Seven of them were redundant even if they had worked: * `HSA_NO_SCRATCH_RECLAIM: 1`, `NVTE_CK_USES_BWD_V3: 1`, `PRIMUS_DETERMINISTIC: 0` already match the launcher defaults (`runner/helpers/envs/base_env.sh:200,240,253`). * `TURBO_DEEPEEP`, `TURBO_GROUPED_MLP`, `LEGACY_GG` are local variables in the `examples/moe_package/*.sh` wrappers, whose only job is to produce `use_turbo_deepep`, `use_turbo_grouped_gemm` and `moe_use_legacy_grouped_gemm` -- and this file already sets all three to the same values further down. * `CLEAN_DOCKER_CONTAINER` is a launcher concern; the unified CLI spells it `primus-cli container --clean` and never reads the variable. The two that did carry intent are recorded as a launch command in the comment that replaces the block: * `GPU_MAX_HW_QUEUES: 4` -- the default is 2. * `USING_AIC: 1` -- a typo for `USING_AINIC`, the flag that runs `runner/helpers/hooks/03_enable_ainic.sh`. Both are on the container passthrough allowlist in `runner/.primus.yaml`, so `--env GPU_MAX_HW_QUEUES=4 --env USING_AINIC=1` reaches the job on either launcher. No behaviour change: the deleted keys were inert, and the two live settings were already absent from every run of this config.
These fields are live but cannot be derived from the upstream backend schemas or the backend-package readers the checker scans: * Top-level `env` belongs to the Primus experiment envelope. `TrainRuntime._apply_config_env()` applies it to `os.environ` before backend initialization, so it must not be validated as a MaxText argument. * `hf_path` is read straight out of the yaml by `01_convert_checkpoints.py`, a posttrain runner hook, so there is no `args.hf_path` access to derive. * `mamba_expand` does not build the model -- `hybrid_mamba_mla_layer_specs.py` hardcodes `expand` in the spec it hands to `MambaMixer`. It remains live checkpoint metadata: Megatron saves the whole `args` namespace, and `convert_mamba_hybrid_to_fla_hf.py` reads the key back to size the FLA model. That converter lives under `tools/`, outside the scanned backend package. Each manual rule names its actual consumer so the checker can flag the entry as stale if that file or key disappears.
The loader only understands `extends` (`primus/core/config/yaml_loader.py`); `bases` is parsed as an ordinary key and thrown away. Consequently neither `mamba_base.yaml` nor `language_model.yaml` has ever been merged into these two presets, and every result measured for them came from the file standing alone on top of the Megatron defaults. Renaming `bases` to `extends` is deliberately not done here because it changes the models: * `mamba_370M.yaml` would gain 144 keys and move eight live settings, including position embeddings, linear biases, 10% dropout, and sequence parallelism. * `minimax_m2.5.yaml` would gain 98 keys and move four live settings, including both position-embedding switches and sequence parallelism. Each file keeps that delta as a comment where `bases:` used to be, so adopting the intended base can be reviewed separately with compatibility and convergence evidence. Removing an ignored directive does not change current behaviour. Five presets covered by the original cleanup no longer belong in this commit: #1007 renamed them and deliberately adopted `extends: [mamba_base.yaml]` on `main`. Rebase keeps those newer definitions unchanged.
There was a problem hiding this comment.
Code Review: AMD-AGI/Primus
Summary
No violations found, 60 files reviewed.
Rules checked: 7 (7 passed, 0 failed).
✅ Passed
All 7 rules satisfied. No architecture concerns found.
Posted by PR Pundit — AI-powered code review grounded in this repo's merge history.
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
1 participant
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 this does
Turns backend schema drift into a CI failure and includes the behavior-preserving cleanup that can land without model-owner decisions:
overrides:bases:directives without adopting their basesModel-scoped findings remain informational because correcting them can change training numerics. A backend whose schema submodule could not be checked still fails.
Current draft status
The branch reduces current
mainfrom 66 unknown keys / 1,136 occurrences to 2 unknown keys / 269 occurrences. CI remains red intentionally until the two decision-bearing cleanups are resolved:check_for_nan_in_loss: one native-Megatron MLPerf config; renaming it changes behavior by disabling the NaN/Inf guardauto_continue_train: 268 occurrences; the implementation disappeared withprimus/modulesin refactor: remove primus/modules and migrate still-used code into core/backends #851, so removing it needs a separately reviewable large cleanupVerification