Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions .agents/contributor-skills/megatron-pr-failure-triage/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,22 @@ uv run --script .agents/nemo-rl-testing-agent/scripts/known_issues.py annotate \
Each failure comes back in one of three states.

**`known`** — already diagnosed, with its fix linked. The entry's diagnosis is
written into the test's comment for you. Do not investigate; do not open another
fix branch. If the fix has been sitting in review, chasing the reviewer is the
useful action, not re-debugging the bug.
written into the test's comment for you, and the status drops to
`fail (pre-existing)`, because an entry only ever exists for a break the PR did
not cause. Do not investigate; do not open another fix branch. If the fix has
been sitting in review, chasing the reviewer is the useful action, not
re-debugging the bug.

That downgrade is also why you must not record a break the PR *did* cause. The
registry is cross-PR memory: an entry excuses this failure on every future run,
including the one where it is somebody's fault. Report those against the author
instead.

Do not lean on `apply_baseline.py` to do that downgrade. It only fires when the
cached baseline failed the same test, and the baseline runs on a stack that
carries pending fixes — so the moment a fix exists, the baseline goes green and
stops covering for anybody. That combination once had three innocent PRs
reported as `fail` with a link to the fix printed directly underneath.

**`STALE`** — the registry claims a fix **and that fix is already applied to the
branch this run tested**, yet the test failed anyway. This is a genuine finding.
Expand Down Expand Up @@ -198,13 +211,26 @@ uv run --script .agents/nemo-rl-testing-agent/scripts/known_issues.py record \
--id <stable-slug> --test <test name> \
--signature "<error_signature verbatim from the results JSON>" \
--diagnosis "One or two sentences a PR author can act on." \
--repo NVIDIA-NeMo/RL --fix-pr <n> # omit --fix-pr if there is no fix
--repo NVIDIA-NeMo/RL --fix-pr <n> \ # omit --fix-pr if there is no fix
--fix-branch <branch> \ # required for a Bridge or mcore fix
--first-seen-megatron-pr <N>
```

Copy the signature verbatim; `normalize()` strips the run-specific parts. Never
hand-write one containing a measured value — `median(...) < 1.1 (measured 3.37)`
will not match the same bug measuring 2.01 next week.

`--first-seen-megatron-pr` records which run surfaced the break, for tracing it
back later. It is not a culprit: the break is usually older than that PR and
often in another repository, so nothing reads it as blame.

`--fix-branch` is load-bearing. It is what carries a **Megatron-Bridge or
Megatron-LM** fix into subsequent runs at all: a
NeMo-RL fix rides the integration branch, but a fix in another repository is
checked out inside the container, and `run_suite.sh` looks the branch up here to
decide. Omit it and the fix exists in review while every run keeps reproducing
the bug it fixes — for as long as review takes.

The registry entry is about the bug. If the investigation also taught you
something about *how to investigate* — a place this skill sent you that was
wrong, a symptom that means something other than what the attribution table
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,11 @@ uv run --script .agents/nemo-rl-testing-agent/scripts/known_issues.py record \

The first makes later runs recognise the failure instead of re-debugging it. The
second puts the fix on the integration branch, so later runs do not hit the
failure at all. Only NeMo-RL fixes ride the integration branch; a Megatron-Bridge
or Megatron-LM fix still needs `--bridge-ref` / `--mcore-ref` on the run that
depends on it, so make the registry entry say so.
failure at all. Only NeMo-RL fixes ride the integration branch. A Megatron-Bridge
or Megatron-LM fix is checked out inside the container instead, and the registry
entry is the only thing that knows which branch that is — so record it with
`--fix-branch`, and `run_suite.sh` will carry it until the fix merges. Leave it
off and the branch you just pushed changes nothing about the next run.

`sync_integration.sh` only picks up branches named `mcore-*-fix` whose PR is
open, which is why the naming convention matters. If it reports
Expand Down
11 changes: 10 additions & 1 deletion .agents/contributor-skills/megatron-pr-reporting/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,22 @@ uv run --script .agents/nemo-rl-testing-agent/scripts/post_report.py \
--results ~/.nemo-rl-testing-agent/pr-5700/l1.results.json \
--results ~/.nemo-rl-testing-agent/pr-5700/l2.results.json \
--integration ~/.nemo-rl-testing-agent/integration.json \
--meta megatron-lm=<HEAD_SHA> --meta merged-with-main=<BASE_SHA> \
--meta cluster=oci-hsg --meta image=nvcr.io/nvidian/nemo-rl:nightly
```

Results files render in the order given, so pass L1 before L2. Add `--dry-run`
to review the markdown first — always do this before the first post on a PR.

Do not pass revisions as `--meta`. The report builds an **Exactly what was
tested** table out of the `prep` block the run recorded, naming the ref *and*
the commit for megatron-core, Megatron-Bridge and NeMo-RL, each linked to the
repository it was actually fetched from. Ref and commit have to travel together:
a bare sha cannot distinguish the Bridge NeMo-RL pins from a fix branch carried
in its place, and a NeMo-RL sha links nowhere useful unless the reader knows it
came from a fork. Hand-typed shas were also a silent source of error — nothing
checked them against the run, so a stale copy-paste read exactly like a result.
`--meta` is for what the run does not record, such as the cluster and image.

**Always pass `--integration`.** Runs carry NeMo-RL fixes that are raised but not
merged, so a green table means "green with those applied", and a reader entitled
to assume otherwise would draw the wrong conclusion. The flag renders one line
Expand Down
11 changes: 11 additions & 0 deletions .agents/contributor-skills/megatron-pr-test-run/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,17 @@ self-consistent: it is the combination NeMo-RL ships and CI expects. Use
`--bridge-ref <sha|ref>` to test a specific Bridge, or `--bridge-ref image` to
keep whatever the image carries (only useful for reproducing an old run).

One case overrides that default on its own. When the registry holds an open
Bridge fix with a `fix_branch`, `run_suite.sh` checks that branch out instead
and logs `carrying unmerged Megatron-Bridge fix '<branch>'`. The pinned Bridge
necessarily lags megatron-core `main` by however long it has been since NeMo-RL
bumped the submodule, so an mcore change to an API Bridge uses breaks every test
at import until the Bridge-side fix merges *and* NeMo-RL advances the pin — a
window of weeks, during which the default pin makes each sweep re-report a break
that is already fixed in review. Read the `BRIDGE_REF=` line rather than
assuming: if it names a branch, the run is not on the shipped combination, and
that belongs in the report note.

## Which NeMo-RL is under test

The third leg is pinned the same way, and by default it is **not** plain `main`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ That does five things, in order:
2. `known_issues.py refresh` asks GitHub about each registry entry's fix PR and
retires the merged ones.
3. `ensure_baseline.sh --force` runs the suite: megatron-core `main` × the
integration branch × the Bridge sha NeMo-RL pins.
integration branch × the Bridge sha NeMo-RL pins, or an open Bridge fix
branch when the registry has one (step 2 is what decides that, by retiring
the entries whose fixes merged).
4. `known_issues.py annotate` labels each failure with what we already know.
5. `post_tracking_issue.py` renders the tracking issue.

Expand Down
79 changes: 78 additions & 1 deletion .agents/nemo-rl-testing-agent/scripts/known_issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,18 @@
``annotate`` says so loudly instead of quietly labelling it known -- the entry
is stale, or this is a different bug wearing the same signature.

An entry does two things beyond naming the bug. It downgrades the failure out of
`fail`, because an entry exists only for a break the revision under test did not
cause. And, for a fix in a repo the integration branch cannot carry, it tells
later runs which branch to check out, so a Bridge fix in review stops having to
be remembered by hand on every submit.

Usage:
uv run --script known_issues.py annotate --results r.json [--integration m.json]
uv run --script known_issues.py record --id <slug> --test <name> \
--signature "<error line>" --diagnosis "..." --repo owner/name --fix-pr 3363
--signature "<error line>" --diagnosis "..." --repo owner/name --fix-pr 3363 \
[--fix-branch <branch>] [--first-seen-megatron-pr <n>]
uv run --script known_issues.py pending-fix-ref --repo owner/name
uv run --script known_issues.py refresh
uv run --script known_issues.py list
"""
Expand Down Expand Up @@ -199,6 +207,26 @@ def cmd_annotate(args: argparse.Namespace) -> int:

test["known_issue"] = issue.get("id")
test["comment"] = build_comment(issue)

# An entry only ever describes a break that is *not* the fault of the
# revision under test -- triage records one after deciding the PR did not
# cause it, and a break the PR did cause is reported against the author
# rather than filed as cross-PR memory. So a match has to move the status
# as well as the comment, since `fail` means "this PR broke it". Leaving
# it alone once put three innocent PRs on the hook: the baseline had been
# taken on a stack carrying the fix, so `apply_baseline` had nothing to
# downgrade, and the report read `fail` with a link to the fix printed
# directly underneath.
#
# Note this deliberately does not consult first_seen_megatron_pr. That
# field records which PR's run first surfaced the break, which is not the
# same as which PR caused it and is usually not even the same repository
# -- the Bridge break below was first seen on Megatron-LM#5382 and caused
# by #5865, long merged. Reading it as the culprit would blame whichever
# PR had the bad luck to run first.
if test.get("status") == "fail":
test["status"] = "fail (pre-existing)"

issue["last_seen_utc"] = now_utc()
matched += 1
fix = f"{issue.get('repo')}#{fix_pr}" if fix_pr else "no fix raised yet"
Expand Down Expand Up @@ -332,6 +360,46 @@ def cmd_list(args: argparse.Namespace) -> int:
return 0


def cmd_pending_fix_ref(args: argparse.Namespace) -> int:
"""The branch a run must check out to carry an unmerged fix in one repo.

A NeMo-RL fix reaches later runs on its own: sync_integration.sh cherry-picks
every open `mcore-*-fix` PR onto the integration branch. A fix in
Megatron-Bridge or Megatron-LM cannot ride along, because the branch is a
NeMo-RL branch and those commits live in another repository -- they are
checked out inside the container instead. Without this lookup the operator has
to remember to pass `--bridge-ref` on every submit, and the sweep that forgets
reports a break that is already fixed in review as though the PR under test
caused it.

Prints one branch name, or nothing when no such fix is pending. Two pending
fixes in one repo cannot be combined into a single ref, so that is an error
rather than a silent pick.
"""
registry = load(registry_path(args.registry))
branches = sorted(
{
issue["fix_branch"]
for issue in registry.get("issues", [])
if issue.get("state") in (None, "open")
and issue.get("repo") == args.repo
and issue.get("fix_branch")
}
)
if not branches:
return 0
if len(branches) > 1:
print(
f"known_issues: {len(branches)} unmerged fixes pending in {args.repo} "
f"({', '.join(branches)}); they cannot be carried by one ref. Merge one, "
"or pass the ref explicitly.",
file=sys.stderr,
)
return 1
print(branches[0])
return 0


def main() -> int:
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("--registry", type=Path, help="Override $KNOWN_ISSUES_FILE.")
Expand All @@ -348,6 +416,15 @@ def main() -> int:
)
annotate.set_defaults(func=cmd_annotate)

pending = sub.add_parser(
"pending-fix-ref",
help="Print the branch carrying an unmerged fix for one repo, if any.",
)
pending.add_argument(
"--repo", required=True, help="Repo the fix lands in, owner/name."
)
pending.set_defaults(func=cmd_pending_fix_ref)

record = sub.add_parser("record", help="Add or update an entry.")
record.add_argument(
"--id", required=True, help="Stable slug, e.g. mcore-5918-prompt-tokens."
Expand Down
3 changes: 2 additions & 1 deletion .agents/nemo-rl-testing-agent/scripts/parse_results.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
PREP_FIELD_RE = re.compile(
r"^(mcore_fetch_ref|mcore_sha|mcore_sha_before|mcore_subject|megatron_core_file"
r"|bridge_fetch_ref|bridge_sha"
r"|nemo_rl_fetch_ref|nemo_rl_sha|nemo_rl_env_sha|nemo_rl_mode)=(.*)$"
r"|nemo_rl_fetch_ref|nemo_rl_sha|nemo_rl_env_sha|nemo_rl_mode"
r"|nemo_rl_url|mcore_url|bridge_url)=(.*)$"
)

# Ordered by how specific the signal is; the first pattern that matches when
Expand Down
Loading
Loading