Skip to content

feat(ci): metric-history gate — GATE_DEFAULTS one-liners + standard-metric sweep (M4)#1630

Open
guapisolo wants to merge 1 commit into
ci-history-gate-m3from
ci-history-gate-m4
Open

feat(ci): metric-history gate — GATE_DEFAULTS one-liners + standard-metric sweep (M4)#1630
guapisolo wants to merge 1 commit into
ci-history-gate-m3from
ci-history-gate-m4

Conversation

@guapisolo

Copy link
Copy Markdown
Collaborator

Depends on: #1517
Stacked on the parent PR's branch. Review / merge the parent first.

Summary

One-line register_ci_gate declarations for standard metrics, swept across the CUDA RL e2e tests.

Motivation

Requiring every test to write steps + constraint literals makes blanket coverage expensive: the standard metrics (train/grad_norm, train/ppo_kl, logp-diff, rollout/raw_reward) want the same defaults everywhere. A per-metric_key defaults table beside the parser supplies them, so the one-liner is a complete minimal declaration. The sweep lands in the same PR so the table ships together with its consumers.

Usage

register_cuda_ci(est_time=600, suite="stage-c-8-gpu-h100")

register_ci_gate(metric_key="rollout/raw_reward")  # steps + constraint from GATE_DEFAULTS

Design Notes

  • Key choices: a defaulted field keys on the TABLE literal (same canonical-JSON derivation as a written literal), so editing a table entry re-keys every declaration relying on it — one global reset lever per standard metric.
  • A written literal always wins over its table default.
  • A metric with no GATE_DEFAULTS entry must still write both fields.
  • Table keys stay within the capture whitelist (test-enforced).
  • The sweep declares all five metrics on each of the 22 targets: the sglang rollout path requests return_logprob unconditionally, every target runs policy_loss, so all five keys exist in every target's record.
  • Deliberately skipped: disabled tests, serving / precision files (no training metrics), test_mimo_7B_mtp_only_grad (zero grads by design), test_qwen3_4B_ckpt (multi-stage record merge needs its own look).

Verification

  • Tests added: test_one_liner_fills_from_gate_defaults, test_partial_default_written_literal_wins, test_one_liner_without_table_entry_rejected, test_gate_defaults_within_capture_whitelist_and_valid, test_one_liner_runtime_is_noop.
  • All 22 swept files machine-verified: each parses into 5 valid specs with the register_cuda_ci registry intact.
  • Full tests/ci/test/ run passes (247 passed, 1 skipped).

Review Focus

  • Scrutinize the GATE_DEFAULTS band values in register.py — shadow-calibration starting points; test owners tune or veto their line in the sweep commit.
  • Scrutinize the defaults fill in _parse_ci_gate_call — it runs before the required-field check, feeding _canonical_key the table literal, never the normalized form.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a defaults mechanism (GATE_DEFAULTS) for standard metrics in CI gates, enabling concise one-liner declarations across various end-to-end tests. Omitted fields are automatically populated from these defaults during parsing. The feedback suggests copying the default values (such as lists or dicts) rather than assigning them by reference to prevent accidental mutation of the global GATE_DEFAULTS dictionary.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +301 to +302
if field not in raw:
raw[field] = defaults[field]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent accidental mutation of the global GATE_DEFAULTS dictionary (or any nested mutable structures like constraint dicts or step lists) by downstream consumers, it is safer to copy the default values rather than assigning them by reference.

Suggested change
if field not in raw:
raw[field] = defaults[field]
if field not in raw:
val = defaults[field]
raw[field] = val.copy() if isinstance(val, (dict, list)) else val

Provide schema-validated defaults for the five standard metrics and use one-line declarations in eligible Megatron CUDA training tests on the current upstream layout.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant