What has to be fixed before #4108 can merge

Companion illustration for NVIDIA-NeMo/RL #4108 — all permalinks pinned to d044f40.

Before
The Energon SFT loader handled one family of multimodal datasets, through the generic cooker and task encoder.
This PR
Adds a second family for Nemotron models — ten cookers, three task-encoder modules, roughly 5,100 new lines.
Status
Open — none of the nine below are fixed. All re-checked against d044f40 after the rebase; all still present. Each has its own review comment, linked from its section.

Three pages on this PR, one question each — this one: the nine things that have to change first, one section each.

The review left fourteen comments. Five are design cleanups that can land afterwards. The nine below stop this from merging: three that a CI job refuses, and six that no job checks at all — those only appear once someone runs a real training job.

Caught by CI — red now, or red the first time tests run

1 The Megatron-Bridge pin points off the main branch

Megatron-Bridge history 5ed97996 what RL main pins Bridge main continues → 3961f399 the PR's pin — head of a feature branch, never merged to main 42 ahead · 93 behind fast-forward check fails

RL's main pins an ordinary commit on Bridge's main line. This PR moves the pin onto 3961f399, which is the head of the chcui/sage/nmvl feature branch — Bridge PR #5894 was merged into that branch, not into main. Taking it adds 42 commits and drops 45, including a Nemotron VL config fix. CI's submodule fast-forward job refuses it.

Fix: say which Bridge changes this needs, land them on Bridge main, then repin to something that fast-forwards from 5ed97996.

🔴 review comment — Megatron-Bridge submodule

2 A personal soak config turns secrets-detector red

sft-super67b-generalist.n16soak.yaml 104: log_dir: /mnt/…/logs/sft-super67b-n16-seq… 109: tensorboard: /mnt/…/logs/tb-sft-super67b-… secrets-detector: FAIL "Base64 High Entropy String" ×2

Both flags are false positives — the detector trips on the long hyphenated path, not on a real secret. But the paths are only there because the file is one person's scratch config: it hardcodes a single filesystem for model, dataset, cache, logs and checkpoints, ships a one-off run name, and leaves the debug flag on. Its commit message is "working config?".

Fix: drop the file. That clears the job with no baseline edit. Regenerating the detector baseline would hide the warning and leave the paths in public source.

🔴 review comment — n16soak.yaml:104

3 29 of the PR's own tests fail, in the lane that collects them

3 new test files all marked pytest.mark.mcore L0_Unit_Tests_Data no --mcore-only → drops them L0_Unit_Tests_Megatron --mcore-only → collects all 29 29 failed, 27 passed lane goes red

Three of us ran the suite in three separate virtualenvs and got identical counts. It is not an environment artifact: two of the five files pass 100% in the same interpreter. Eighteen of the failures come from one cause — a new @stateless(restore_seeds=True) makes preencode_sample unusable outside a live Energon worker, and the tests call it directly. Five are source bugs with their own sections below.

Fix: run the suite locally, then push and comment /ok to test. For the eighteen, an autouse fixture activating a WorkerConfig beats editing 25 call sites — the same pattern this PR already uses elsewhere.

🔴 review comment — the failing suite

Nothing checks these — they appear during a real run

4 A broken dataset never stops the job

every sample fails to decode — wrong prompt_format, or the wrong processor fail fail fail fail … forever handler prints, returns, counter climbs — nothing ever raises was: tolerance 1 → raised here now: tolerance 0 → check switched off

Energon counts consecutive failures and raises past a limit. This PR sets that limit to 0, which Energon reads as "stop counting" — its own docstring says so, and its own default is 100. The same number also switches off a second stop in the packing loop. The run is noisy, not quiet: it prints per failure, including a failed N/0 times in a row counter that can never trip. It simply never ends. The PR also deletes the comment that explained all this.

Fix: set it to 100 and restore the comment. Energon already has SkipSample for dropping bad samples cleanly, so 0 was never needed for that.

🔴 review comment — generic_sft.py:259

5 An omni run trains with no audio, and says nothing

An "omni" model reads text, images and sound. Turning a sound file into something it can read takes three settings, and they come from the processor — the object that converts raw media into model inputs. The three are: what rate to resample the audio to, the step between analysis windows, and how much the audio part of the model shrinks the result. A processor built only for text and images carries none of them.

the processor it was given resample to— window step— shrink by— text-and-image only: all three missing at startup all three present? → no answer is never used to stop on every audio sample all three present? → no raises — but far too late the run starts nothing complains caught, printed, sample skipped audio gone the same question, asked twice — only the late one does anything, and by then it is swallowed job finishes normally: text and images trained, every sound file dropped

Those three settings are checked in two places, and the checks are identical. At startup the loader asks whether all three are there, gets "no" — and then uses that answer only to decide whether to run an unrelated check on one of the numbers. It never refuses the run. The very same question is asked again on every audio sample, deep inside encoding, and there it does raise. By then it is useless: the error is caught, printed, and the sample is thrown away, and because the tolerance in section 4 is 0 it never adds up to a stop.

So a run pointed at the wrong processor starts cleanly, prints a line per sound file, and finishes having trained on the text and the images with every piece of audio missing. The PR's own test tries to catch this — it blanks one of the three settings and expects startup to fail — and it currently reports DID NOT RAISE.

Fix: refuse at startup when any of the three is missing. The loader has already worked out the answer; it just throws it away. Fix the code rather than deleting the test, since deleting it ships the quiet half-run.

🔴 review comment — nemotron_multimodal.py:460

6 Every media file is decoded, then thrown away

opened = _open_media(…) runs first, always if not metadata: decides if it was needed if cache is None: return raw `opened` discarded — work wasted the encoder's own comment says payloads stay lazy until a sample is selected

The file is opened before anything decides whether it is needed, and the result is dropped when there is no cache pool. On a WebDataset run without one, that is every image, video and audio member of every sample — and for a path-valued member it hits the filesystem during cooking. Two of the 29 test failures land here.

Fix: open only on the paths that use the handle. Do not just move the call below the cache check — the metadata step also consumes opened, so a straight move breaks it.

🔴 review comment — cookers/nemotron.py:371

7 A conversation ending on a user turn crashes

boundaries = positions of the turn marker, one per user AND assistant turn [0] 5 [1] 19 [2] 34 [3] — none guard: len < 2 → ok, 3 passes loop reads boundaries[3] → IndexError system turns use a different marker, so the count is #user + #assistant — odd exactly when the last turn is a user turn with no assistant reply

The guard rejects fewer than two markers. The loop then walks the list and reads boundaries[index + 1] on even positions, so three markers means it reaches for a fourth that is not there. Unusual in curated data, entirely normal in a scraped corpus. This is a loud crash, not corrupted training — the point is that a guard already exists and catches too few cases.

Fix: reject an odd count in the guard that is already there — one condition added to the existing raise.

🔴 review comment — nemotron_tokenization.py:399

8 Three shipped recipes stop loading

recipes still on the old shape qwen2.5-vl…packing.yaml: 64 nemotron-omni…tp8ep8: 64 …tp4ep8cp2: inherits it packing_buffer_size: None only null is accepted now ValidationError at config load

The field was int | None; it is now typed so that only null passes. That is deliberate — the PR's own test asserts an integer is rejected — but the recipes were not moved to the replacement task_encoder.packing block. Two set the value directly and a third inherits from one of them. No CI job catches it: the functional test runs the non-packing recipe, and these three are disabled for GPU-hour budget with a note saying to run them by hand.

Fix: move the two files onto task_encoder.packing. The docs guide still describes the old field too, and needs the same edit.

🔴 review comment — energon/config.py:154

9 Audio runs die once training has already started

import librosa inside the audio path declared in… ✗ pyproject dependencies ✗ any optional extra ✗ install_audio_deps.sh ModuleNotFoundError mid-run, inside the loader

The import is bare, so the user sees a raw ModuleNotFoundError from inside the loader rather than a message naming what to install. It is not only the resampling branch: per the comment two lines above it, the feature extractor needs librosa too, so any audio path needs it. The only thing that installs it today is this PR's own dev script, which no CI path runs. soxr is missing the same way.

It does show up in the lock file, which is misleading if you grep for it — it resolves there through an optional audio extra of Megatron-Bridge and through TensorRT-LLM, while the project asks for Megatron-Bridge's te and ssm extras instead. So it is resolved in the lock and never installed on this path.

Fix — one word. Megatron-Bridge already packages this: its optional audio extra is exactly ["librosa"], and its own Nemotron Omni code imports librosa. So ask for the extra instead of declaring anything new — megatron-bridge[te,ssm] becomes megatron-bridge[te,ssm,audio] — then refresh the lock file. soxr needs no entry; librosa requires it.

Not energon, and nothing to file upstream. Energon's audio surface is decoding only — its av-decode extra is just av, and librosa appears in none of its extras. Energon reads bytes and decodes them; resampling to one model's rate belongs to whoever owns that model, which is why the extra already lives in Bridge. And not the audio install script: that exists because the av wheel ships FFmpeg itself — libavcodec, libavformat, libavutil, libswresample, libswscale — which is what makes redistributing it a licensing question. librosa is pure Python and bundles no binaries, so that reason does not apply.

🔴 review comment — nemotron_multimodal.py:679

What to do with this. The first three are the gate: two are red in the checks list today and the third goes red the first time anyone comments /ok to test. The other six are the reason to care beyond the gate — no job checks them, so merging with them in place means the first person to run a real Nemotron training job finds them. Five further comments on the PR are design cleanups and can land afterwards.