Commit b090c98
authored
feat(customization): accum every training metric as time series (#1289)
* refactor(rl): use the shared training progress callback directly
RL carried a standalone TrainingProgressCallback that duplicated the shared one
in packages/nmp_customization_common, minus its metric accumulation. Fixes to
progress reporting therefore had to be made twice or -- in practice -- only
once, in whichever copy the author happened to be looking at.
The copy is deleted outright rather than replaced by a subclass, because a
subclass would add nothing: `_default_backend` is already None on the base, and
that default is what keeps RL's status-detail shape unchanged on the wire (no
`backend` key is added). automodel imports the shared class directly for exactly
this reason; unsloth is the only service that subclasses it, and only to stamp
`backend="unsloth"`.
Two additive changes to the shared class make it a drop-in for what RL's copy
supported:
**additional_metrics backend-specific scalars alongside loss/lr/grad_norm.
Splatted first, so a backend metric cannot shadow the
accumulated series or the step's own loss; every other
colliding name is a real parameter and already errors
at the call site.
optional val_loss not every algorithm produces one. The key is omitted
rather than sent as null, which would chart as a zero.
Also adds the missing services/rl/.../training/progress.py, matching the unsloth
and automodel modules that bind SERVICE_NAME, so the two RL construction sites
stop passing it by hand.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): stop non-step reports from erasing the metric series
`report_running` REPLACES the task's status_details blob rather than merging
into it, so a report that omits `metrics` erases the accumulated series from
stored status until the next train step resends it -- and loses it outright if
the job dies in that window.
report_training_start, report_epoch_end and report_checkpoint_saved all omitted
it. automodel calls both report_epoch_end and report_checkpoint_saved
mid-training, so this was reachable in practice, not theoretical. On
report_training_start it also blanked a resumed job's seeded series before the
first step could restate it.
Two automodel tests and one unsloth test pinned the buggy payload with
exact-kwargs assertions; they now assert the series survives instead.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* feat(customization): accumulate a time series for every reported metric
Only train_loss and val_loss were series; every other metric a backend reported
rode as a current-step scalar that the next update overwrote. So the only thing
a finished job could be charted on was its loss, no matter how much else the
backend knew.
Now every numeric metric accumulates into its own series in the same
{step, epoch, value} shape Studio already renders. The current-step scalars stay
on the blob alongside, so consumers can read either the curve or the latest value.
Series are namespaced by phase: train_<name> / val_<name>. The prefix is
load-bearing, not cosmetic -- backends report the same metric name in both their
train and validation dicts (NeMo-RL does this with truncation_rate, and DPO with
accuracy), so unprefixed names would interleave two different quantities into
one curve. train_loss and val_loss keep their bare names, so the existing Studio
loss chart is unaffected.
lr and grad_norm accumulate too; they are curves people read, and they were only
excluded because they happen to be named parameters rather than
**additional_metrics.
fetch_current_metrics had to stop hardcoding the two names, or a resumed job
would silently restart every other curve from empty. It now returns whatever
list-valued series are stored.
The numeric guard lands here as is_chartable(), and NemoRLLogger's
has_metric_value delegates to it: a metric the logger forwards must be one the
callback can chart, and letting those drift is how a histogram object ends up in
a series. It also removes a latent crash -- math.isnan raises TypeError on the
non-scalars a framework metric dict can carry.
Size scales with the number of *reports*, not training steps, since backends
throttle reporting. Measured for a 22-series RL run:
500 steps, log_interval 10 -> 42 KB final blob, 1.1 MB uploaded
500 steps, log_interval 1 -> 413 KB final blob, 101.3 MB uploaded
Accepted for batch training jobs. A backend that reports every step of a long
run pays quadratically; if that becomes a real configuration the fix is delta
appends in the transport, not trimming the series here.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): carry sticky status_details fields across updates
status_details is REPLACED on every update, so a field survives only as long as
the next report repeats it. Three kinds of field were being lost to that:
metrics erased by the runner's checkpoint/completion/failure
reports, which come from a different process than the
training driver and hold no series to resend -- so
every job ended by erasing its own curves, worst of
all on the failure path where the partial curve is
worth the most
max_steps, num_epochs stated once by report_training_start, gone from the
first training step onward
checkpoint_path published by one report, wiped by the next
Studio reads max_steps and checkpoint_path straight out of status_details, so
"step / max steps" fell back to a bare step number for the whole run, and the
latest-checkpoint row appeared and vanished.
_CARRY_FORWARD names the rule: what stays true after the update that stated it.
Cumulative (metrics), run constants (max_steps, num_epochs), monotonic progress
(step, epoch), and sticky latest-values (checkpoint_path).
Excluded deliberately: `phase`, which every report sets for itself, and the
per-step observations (train_loss, lr, grad_norm, ...). Those describe one
instant and a stale copy would misrepresent "current" -- and nothing is lost,
because each is now recoverable from its series. percentage_done is excluded
too: it is derived from step and max_steps, both carried, so a consumer can
recompute it rather than risk a copy that contradicts its own inputs.
Keeping the GET off the hot path is the design constraint. Values are remembered
as they pass through, so a process that has already stated a field restates it
for free; the stored blob is read back only when an update omits `metrics`,
which is the tell that it did not come from TrainingProgressCallback. Per-step
reports always carry `metrics` and never fetch. The runner's handful always do.
One subtlety: on resume the driver's first report already carries `metrics`, so
it would never read the blob back and would drop the previous run's
checkpoint_path. _fetch_status_details therefore refreshes the cache as a side
effect, which makes the resume-seeding fetch the callback already performs at
construction double as the carry-forward seed -- no extra round-trip.
Tests drive the SDK client seam rather than stubbing the fetch, so the real
_fetch_status_details runs, cache side effect included. First test coverage for
this module.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(rl): report the final training step, and stop double-counting it
Two defects in NemoRLLogger, both in how it counts and reports steps.
The final training step was never reported. The throttle is `step %
log_interval == 0`, so when max_steps is not a multiple of log_interval the last
steps are dropped -- at 23 steps and an interval of 10 the run's last recorded
loss was step 20's. A withheld step is now held as pending and flushed by
close().
Nothing called close(). The driver appends the logger to `logger_inst.loggers`
and never tears it down; nemo_rl.utils.logger.Logger has no close() at all --
its only teardown hook is finish(), dispatched as
`getattr(logger, "finish", None)`, which skipped us because NemoRLLogger did not
define one. And dpo_train never calls finish() either; the only caller upstream
is the single-controller path. So the flush would have run only from __del__, at
GC or interpreter shutdown, where every failure is swallowed. Two hooks now,
because neither alone is sufficient: finish() aliases close() under the name the
composite dispatches, and the driver calls close() from a finally, which is the
case that matters -- an abnormal exit is exactly when the last step is worth
having.
Steps were double-counted. `log_metrics` opened with `step = step + 1`, but the
caller already counts from 1: dpo.py logs `total_steps + 1`, where total_steps is
0-based and incremented *after* the log. A 23-step run therefore recorded steps
2..24 against max_steps=23, and the log_interval throttle fired on true steps 9,
19, 29 -- withholding the last step even when max_steps *was* a multiple of the
interval. Epoch derivation read the same inflated step and flipped an epoch early
at the boundary; it now clamps at zero, because step 0 does arrive, from the
validate-at-start path, and belongs to epoch 1.
for_schedule owns the log_interval and steps_per_epoch arithmetic that the DPO
driver used to derive inline. Its `(val_period // 10) + 1` had a `+1` that was a
divide-by-zero guard and also skewed every value it produced, and it raised
outright when val_period was None. DPO's reporting cadence changes slightly as a
result.
The driver teardown is asserted against the AST -- the drivers cannot be
imported outside the training image -- with the detector's own negative cases
pinned, since a tripwire that cannot trip is worse than none.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* thanks CodeRabbit
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(customization): drop the carry-forward machinery
The Jobs service MERGES task status_details key-wise rather than
replacing the blob -- JobDispatcher._update_status_details_object,
applied both to the task and to the copy propagated up to the job. A
field therefore survives every later update that does not restate it.
Verified end-to-end against a running platform: a full mid-training
report followed by a bare {"phase": "processing_checkpoint"} leaves the
series, the schedule and the checkpoint path stored intact. Nothing was
ever erased.
That makes _CARRY_FORWARD, the read-back GET and the metrics payload on
the training-start / checkpoint / epoch-end reports redundant. Removing
them also drops a network round-trip from every non-step report,
including report_error, where it sat between the exception and the error
being recorded.
The merge is shallow, so a report that does send `metrics` still
replaces the stored series wholesale -- the train and validation reports
keep resending every series in full. Dropping the payload from
report_training_start closes a real hole while it is at it: when the
seeding fetch failed, that report wrote an empty accumulator over a
resumed job's stored curves.
Two smaller fixes in the blast radius: fetch_current_metrics copies each
point list so the callback's accumulator no longer aliases the response,
and is_chartable's docstring no longer claims NaN/Inf reach the wire as
bare JSON tokens -- the SDK coerces both to null, so the cost of letting
one through is a hole in the curve, not a malformed blob.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): correct three defects in the reported payload
All three reproduced against a live platform before fixing.
The unprefixed-name exemption was keyed on the metric name alone, so a
backend reporting `val_loss` among a *train* step's metrics appended it
to the validation loss curve -- the exact cross-phase interleaving the
prefix exists to prevent. It is keyed on (phase, name) now, so such a
metric lands in `train_val_loss` and the curve Studio draws as the
validation loss stays clean.
A non-chartable metric was dropped from its series but still splatted
into status_details, where a Histogram makes the whole update fail to
serialize. update_task swallows that error, so every metric in the
report was lost while the job went on looking healthy -- the opposite of
what _record's docstring promises. additional_metrics are now filtered
once and the filtered set feeds both the series and the payload, so a
metric rides along as a current-step scalar exactly when it entered a
series. A metric named `phase` goes out through the same filter: it
collides with report_running's own parameter and raised TypeError into
the training loop rather than being shadowed by splat order.
train_loss, lr, grad_norm and val_loss are now stated only when
observed, which is what val_loss already did alone. An absent lr or a
NaN grad_norm -- routine on a skipped step -- otherwise reached the
server as a null, and a chart reads null as a real zero.
Also hardens is_chartable against the OverflowError float() raises on an
unbounded int: it was the one input that could still raise out of a
predicate whose two call sites both rely on it never raising.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* chore(rl): give the new progress module the header its siblings use
The file was added with an Apache-2.0 SPDX identifier followed by an
NVIDIA proprietary "any use ... is strictly prohibited" clause -- two
mutually exclusive licenses on one file -- plus a 2026-only copyright
year. The block came from the deleted backends/nemo_rl/callbacks.py.
Its own docstring says it mirrors the equivalent modules in the unsloth
and automodel services; both of those, and its directory neighbour
runner.py, use the plain two-line 2025-2026 Apache-2.0 header. Match
them.
Scoped to the file this branch adds. The same block sits on 17 other
files under services/rl and services/automodel, which is a pre-existing
repo-wide question rather than this PR's to answer. Note that
check-copyright-headers cannot catch any of it: the fixer only adds a
header where one is missing and never inspects an existing one, so all
6210 files currently report as correct.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* docs(customization): stop justifying the design with an unwired GRPO path
grpo_driver.py is a 108-line stub on this branch: it never constructs a
NemoRLLogger, so no GRPO run reports anything. The docstrings added here
nonetheless leaned on GRPO behaviour to explain three decisions -- the
phase prefix (`truncation_rate` in both metric dicts), the optional
val_loss (validating on accuracy/avg_length with no loss), and the
payload measurements ("GRPO's ~22 series"). A reader on this branch
cannot check any of it.
Restated against what is actually wired. DPO's `accuracy` already
appears in both its train and validation dicts, so it carries the
prefix argument on its own; the optional val_loss is explained by the
general case rather than one algorithm; and the payload numbers are real
measurements, now attributed to "a backend reporting ~22 series" instead
of to a path that does not run.
Two stale references fixed while in here: the step-indexing comment cited
nemo_rl/algorithms/grpo.py as a second caller when only dpo.py calls in,
and two test docstrings pointed at a sibling named test_grpo_config --
the file is test_dpo_config.py. The test that pins cross-phase series
separation now uses `accuracy`, the collision that actually occurs,
rather than GRPO's `truncation_rate`.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(rl): make the steps_per_epoch fallback reachable, and drop a string hint
for_schedule takes `steps_per_epoch: int | None = None` and derives the
value from max_steps and num_epochs when it is missing. That fallback
could never run: dpo_driver read `config.dpo.steps_per_epoch` as a plain
attribute, and the field is an undeclared extra that exists only because
DPOConfig allows extras. pydantic raises AttributeError for a missing
extra, so a config compiled anywhere other than dpo_config.py crashed at
driver startup -- the exact failure the fallback was written to absorb.
Read with a defaulted getattr instead.
Guarded with an AST tripwire alongside the existing close()-in-finally
one, for the same reason that file gives: the drivers pull in nemo_rl and
omegaconf at module scope, so they cannot be imported in a unit test and
a regression here would be silent.
Separately, for_schedule's return annotation was the string
"NemoRLLogger". AGENTS.md asks for concrete hints over string-based ones;
typing.Self is the concrete form for a classmethod constructor, and
matches NMPJobContext.from_env.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* docs(customization): keep the callback's rationale out of the reporter
progress.py was explaining why TrainingProgressCallback resends whole
series and why some reports omit the metrics key. The dependency runs
the other way -- the callback composes the reporter, not the reverse --
so the reporter should state the transport property and stop there.
progress.py now says only what it owns: the service merges key-wise and
the merge is shallow. The consequence for the accumulator moves into
callbacks.py, next to the code that acts on it.
No behaviour change.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(rl): stop holding a reference the logger never reads
self._reporter was assigned and never touched again. The reporter exists
only to be composed into TrainingProgressCallback, which owns it from
that point: close() reaches it through self._callback.close(), not
through the logger.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(customization)!: one naming rule for every training metric
train_loss and val_loss were special: named parameters on the callback,
exempt from the phase prefix, recorded and forwarded by a different code
path than the `**additional_metrics` bag. lr and grad_norm were a third
case -- named parameters, but prefixed like ordinary metrics in the
series and unprefixed at the top level. Four treatments for four kinds of
the same thing.
There is now one. A backend hands over its framework's metric dict under
its own names, and `<phase>_<name>` is the stored series name AND the
current-value key. `train_loss` and `val_loss` are what that rule
produces for a metric called `loss`, which is why the two series Studio
charts did not have to move -- the special case existed only because
callers passed them pre-prefixed.
Prefixing also retires a whole bug class rather than filtering it. A
metric named `phase` used to raise TypeError into the training loop, and
`step`/`epoch`/`metrics` needed splat ordering to avoid being shadowed;
none of them is reachable from a `<phase>_` name, so _RESERVED and the
ordering comments are gone.
The metric bag is now a Mapping parameter rather than **kwargs. Backends
forward whatever their framework emits and a framework is free to call
something `step`, which as **kwargs was a hard TypeError.
BREAKING: top-level `lr` and `grad_norm` in status_details are now
`train_lr` and `train_grad_norm`; Studio is updated to match. The series
payload and the top-level `train_loss`/`val_loss` are unchanged.
Also drops NeMo-RL's metric allow-list. The callback already keeps the
finite scalars and drops the rest, so the list was a second gate doing a
weaker version of the same check -- and it silently dropped DPO's
accuracy, sft_loss and rewards_chosen_mean for never having been added to
it. NeMo-RL's dict is forwarded whole, so a metric it adds charts without
a change here. has_metric_value, _select_metrics and the
_VALIDATION_METRIC_KEYS alias go with it.
Verified against a running platform: train_loss/val_loss keep their
names, train and val `accuracy` stay separate, the DPO scalars the
allow-list dropped now chart, a Histogram and a nested dict are dropped
without costing the report, and metrics named phase/step/metrics land as
train_phase/train_step/train_metrics with the real fields intact.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* docs(customizer): document the metric naming rule and the series payload
Both pages described a fixed pair of metrics and a flat set of
status_details fields, which stopped being true when every reported
metric started accumulating as a series, and stopped being accurate at
all when `lr` and `grad_norm` became `train_lr` and `train_grad_norm`.
get-job-status now lists the progress fields, the per-metric latest
values and the `metrics` history separately, and both example responses
carry a real `metrics` payload rather than only the flat scalars.
The metrics tutorial gains the naming rule -- `<phase>_<metric>`, with
train_loss and val_loss as what it produces for `loss` -- plus where each
metric appears and why a missing field is not a zero. Its API sample
reads the renamed fields and gains a loop over `metrics`, which is how a
caller picks up backend-specific curves without naming them in advance.
optimize-throughput.mdx needed no change: it reads train_loss and
val_loss, and neither name moved.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(rl): bound progress reports by run length, not just val_period
resolve_log_interval derived the reporting cadence from val_period alone,
targeting ~10 reports per validation period. val_period is the user's
val_check_interval, so any value below 10 -- "validate every 5 steps" is
an ordinary request -- floored the term to zero, clamped to 1, and
reported every step of a run of any length.
That is not a linear cost. Every train report resends every accumulated
series in full, and the Jobs service persists each one twice (the task,
then the copy propagated up to the job), so upload and stored-blob writes
both grow as the square of the report count. A 20k-step run at ~22 series
was reporting 20,000 times.
There is now a second floor at _MAX_REPORTS_PER_RUN reports for the whole
run, and the coarser of the two wins. Ceiling division, so the bound is a
real <=200 rather than up to twice that. Nothing in the existing regime
moves: val_period=100 over 100 steps still gives an interval of 10.
The payload note in callbacks.py is corrected while it is being cited.
Its measured figures are client-side upload only, and it claimed that a
backend reporting every step of a long run was a hypothetical the
transport would have to grow delta appends for -- it was reachable from a
documented hyperparameter, and a backend can simply bound itself.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(automodel): strip the val_ prefix the recipes already applied
The shared callback's naming rule is that a backend passes its
framework's own metric name and the phase supplies the prefix. Automodel
is the one backend whose framework prefixes some of its validation
metrics itself, so a name arriving pre-prefixed came back doubled.
That was handled by renaming exactly `val_loss` to `loss`, which fixed
the one curve Studio charts and left every other prefixed name alone:
train_bi_encoder reports val_acc1 and val_mrr, which landed as
val_val_acc1 and val_val_mrr.
strip_val_prefix takes the prefix off wherever the recipe happened to put
one. It has to be unconditional rather than a list, because the recipes
are inconsistent about which metrics carry it -- train_ft pairs `val_loss`
with a bare `lr`, `num_label_tokens` and `mem`, all of which still pick up
the phase prefix normally. removeprefix, not a replace, so an interior
`val_` stays part of the name.
finetune.py imports the recipes at module scope and nemo_automodel exists
only inside the training image, so the test stubs the six leaf modules to
import it. Through monkeypatch.setitem rather than the module-scope
sys.modules assignment its neighbours use: those outlive the file that
installed them and leak into whatever shares the xdist worker, which is
already why two unsloth tests fail whenever this directory runs first.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): discard the steps a resumed run replays
Reporting runs ahead of checkpointing, so the two do not line up: a job
that checkpoints at step 100 and is interrupted at 150 has already
reported 110 through 150. Resuming rolls training back to the checkpoint
and replays those steps, and the accumulator -- seeded from the server
precisely so a resumed job continues its curves -- appended the replayed
points after the ones they superseded. The curve then doubled back on
itself, with two values at each replayed step.
A report below the high-water mark is what identifies that: training only
ever moves forward within a run, so a step behind the furthest one
recorded means a rewind. Every point from that step on is dropped before
the new one lands. The replayed values are the real ones; what goes is
work that was rolled back, and the gap that leaves is honest where the
doubled-back curve was not.
Across every series, not only the one being written. Validation runs on
its own cadence, so pruning per-series would leave a val curve carrying
rolled-back points until the next validation pass -- hundreds of steps
later, or never on a short run.
The comparison is strict. NeMo-RL validates at step N before logging
train N, so a report that merely fails to advance the mark is not a
rewind; treating it as one would have the train report delete the
validation point that legitimately shares its step. A rewind to 0 is a
task rerunning without a checkpoint, and clearing the curves is right.
Stored points are read back from a blob this process did not write, so
seeding now drops any that record no step: unplaceable on a curve and
unplaceable against a rewind. That also keeps the new comparison total --
report_train_step is called straight from NeMo-RL's log_metrics with
nothing catching underneath, so it must not raise on a malformed point.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(rl): report a validation pass that scores on something other than loss
The validation branch required a chartable `loss` before it reported
anything, which is the last place the old "train_loss and val_loss are
special" assumption survived. GRPO scores validation on rewards: its
validate() returns `accuracy` and `avg_length` and no loss at all, so the
gate dropped not its loss curve but every validation pass it ran.
It now reports whatever the pass produced, gated only against a hollow
report -- a pass whose metrics are all histograms still says nothing.
Best-so-far still tracks the validation loss and so is updated only when
there is one.
The train branch keeps its `loss` check, which is doing different work
and is worth saying so explicitly. GRPO and PPO log twice under
`prefix="train"` at a single step -- rollout stats first, then the
training metrics, both at `total_steps + 1` -- and only the second
carries a loss. There it is a discriminator, not a requirement: without
it one step reports twice, each report resending every series, and a
throttled step ends up pending as the rollout half with the loss lost.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): correct twelve defects across the progress reporting paths
Everything a review of this branch turned up, in the shared callback and in all
three backends that call it. Grouped below by the assumption each set breaks,
because several are the same mistake wearing different clothes: a report says
more than it observed, or arrives somewhere the code downstream did not expect.
Seeding
-------
fetch_current_metrics parsed the stored blob outside its try, so a
status_details not shaped like {"metrics": {...}} raised AttributeError rather
than seeding nothing. It runs from TrainingProgressCallback.__init__, which
AutomodelRecipeWrapper and NemoRLLogger both build outside any try, so the
training subprocess died at startup over data that should have cost it only its
curves. Both the blob and its metrics entry are checked now.
A read that failed and a read that found nothing both answered {}, leaving the
caller no way to tell them apart. The server replaces a sent key wholesale, so
seeding from {} after a failed read had the first train report replace the job's
entire stored history with the one point this process held; a distributed launch
racing a briefly unreachable Jobs service lands exactly there. Only a 404 now
means "nothing stored"; every other failure warns and returns None, and the
callback withholds the metrics key for the life of the process. This run's
curves stall, which a later run recovers, rather than every previous run's being
destroyed, which nothing recovers.
Not addressed, and now written down where the seeding is: a series can hold
points from more than one run. A replaced pod seeds itself from the old run's
points and, nothing being able to restore a checkpoint, starts again at step
one, so two values can sit at one step contradicting each other. Both fixes are
bigger than they look -- a run identifier per point needs Studio to filter or
overlay by run, and dropping the superseded points loses a failed attempt's
history for good -- so the behaviour is left as it is, described in the module
docstring and pinned by a test rather than left for someone to trip over.
What a report states
--------------------
report_training_start sent a literal step: 0. report_running derives
percentage_done from a stated step and the merge is wholesale per key, so that 0
overwrote whatever progress the task had stored, in a field harder to spot than
the metrics it already omits because the epoch beside it is not restated and
goes on reading correctly. It fires before the first step and has no position to
report, so it states none and lets the first train step do it.
A report whose values were all unchartable resent every series anyway. They go
in full or not at all, so a report that added no point has nothing to say about
them -- up to 413 KB, by this module's own measurements, to say nothing.
The four-line stamp-and-send tail every report repeated is now one _send.
_default_backend is None for two of the three backends, so a change to it that
missed a copy would have shown up only in unsloth's payload.
Report cadence and cost
-----------------------
_MAX_REPORTS_PER_RUN capped train reports and nothing else, so it did not bound
a run. val_check_interval=1 is reachable, and it validates every step: a
20k-step run made 200 train reports and 20,000 validation ones, each resending
every series in full and stored twice by the Jobs service -- the quadratic
growth the cap exists to prevent, arriving through the other door. The same
bound now applies to validation, counted in passes because passes come on their
own cadence. It resolves to "every pass" for any ordinary configuration, so
nothing in the existing regime moves. A withheld pass is held pending and
flushed by close(), as the train path already did, since the final validation is
the one worth having.
Metric naming, per backend
--------------------------
NeMo-RL's validate() logs once per dataloader, every call at the same step under
`validation-<name>`. Forwarded as-is, two datasets' loss landed as two points at
one step in a single val_loss series -- the collision the <phase>_ rule exists to
prevent, one level further down. The first prefix seen keeps the bare names,
because NeMo-RL names the dataloader even when there is only one, and
disambiguating unconditionally would rename the common case and take Studio's
curve with it.
Automodel's strip_val_prefix maps every name onto its unprefixed form, so a dict
carrying both val_loss and loss collapsed them onto one key and the second
silently replaced the first. The prefixed name wins now, and the collision is
logged.
Unsloth kept float() on loss and dropped it from learning_rate and grad_norm, so
a value the Trainer logs as anything but a numbers.Real -- some paths log
grad_norm as a 0-dim tensor rather than calling .item() -- failed the chartable
filter and vanished from both the series and the report, with no log line saying
why.
Dead state
----------
_best_metric_value and _best_epoch go, with the two tests that pinned them.
Nothing has ever read them; they predate this branch, which was extending
write-only state.
Test isolation
--------------
Two module stubs outlived the files that installed them and leaked across the
xdist worker. automodel's test_config set sys.modules["transformers"] to a
MagicMock at import scope, and unsloth's bridge does `from transformers import
TrainerCallback` at call time -- so HfTrainerProgressCallback became a mock
subclass whose hooks did nothing, and two of its tests failed or passed
vacuously depending on collection order. The RL logger test's nemo_rl stub had
the same shape, which its own docstring conceded. Both are now scoped to the
window that needs them.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(rl): derive the train report cadence from run length alone
resolve_log_interval took the coarser of two floors: ~10 reports across one
validation period, and _MAX_REPORTS_PER_RUN reports across the whole run.
The first has no bearing on the question it was answering. How often
someone wants the loss curve and the progress bar to move is unrelated to
how often the run validates, and the val_period term decided the interval
on nearly every real configuration, so the cap almost never engaged.
Its effect was to hold the report count at ten per epoch at every scale.
compute_val_check_interval returns steps_per_epoch when the user sets no
val_check_interval, and returns it from an early branch that the later
clamps never reach -- so on the default path val_period *was* the epoch.
A one-epoch run drew its whole curve from ten points whether it was 32
steps or 20,000. report_running derives percentage_done from the step a
train report states, so the progress bar advanced ten times too: once an
hour on a 20k-step run at two seconds a step.
The coupling also ran the wrong way. val_period is capped at
steps_per_epoch, and a larger one meant a coarser interval, so choosing to
validate less often -- which is what you do when validation is expensive --
made the training curve worse.
_MAX_REPORTS_PER_RUN is now the only floor, and resolution is flat across
three orders of magnitude: 32 steps -> 32 points, 3,125 -> 195, 20,000 ->
200, against ten for each of them before. Runs at or above 20 epochs were
already governed by the cap and do not move, nor do runs shorter than the
budget, which report every step either way.
The cap keeps its value and gains the justification it should have had.
Two hundred is roughly the number of points a chart a few hundred pixels
wide can draw distinctly, and past which the extra points cost more than
they show. That it also bounds a payload growing as the square of the
report count is why it is a ceiling rather than a target -- and it is an
argument that survives the transport learning to append deltas, where the
cost argument alone would have retired with it.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* update docstrings to reflect seeding mechanism
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(customization): rename the metric-name qualifier off "namespace"
_namespace read as one of four established meanings of the word before it
read as the metric one. NamespacedModel and __schema_namespace__ sit one
module away in the same package and also mean "prefix a name", but for
pydantic schema class names rather than metric keys; the platform's own
resource scoping (parse_resource_id("default/my-model")), pydantic's
protected_namespaces, and argparse.Namespace are the other three.
_qualify_metric_names says which names and what happens to them. The local
at its call site goes from `namespaced` to `qualified`, which matters most
at `if not self._seed_unavailable and qualified:` -- the condition being
tested is "did anything survive the chartable filter", which the new name
states and the old one did not.
NemoRLLogger._namespace_validation becomes _qualify_by_dataset: the same
verb one level down, where the phase qualifies a metric name and the
dataset qualifies it further on a run with more than one validation
dataloader. Renaming one and not the other would have been worse than
leaving both alone.
Four docstrings and two test docstrings follow the same word. The three
remaining uses of "namespace" in these packages -- model_namespace on the
model-entity path, NamespacedModel in the schema tests -- are the other
meanings, and keeping them distinct is the point of the rename.
No behaviour change: identifiers and prose only.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): bound progress reporting for every backend, not one
TrainingProgressCallback consolidated metric accumulation for all three
backends but not the decision of how often to report, which stayed in each
backend and was only made in one of them. NeMo-RL held itself to 200 reports
per path; automodel and unsloth reported every step.
That is not a linear price for verbosity. Every report resends every
accumulated series in full, so the blob grows as series x reports and the
cost as the square of it -- and one report costs three writes, not one.
JobDispatcher persists it to the task and propagates a copy to the job
attempt, and both go through EntityClient.update, which PUTs the entity's
whole data blob. The two server-side writes therefore carry the entire
accumulated metrics whether or not the report mentioned it.
Confirmed against six real completed jobs: train_loss holds exactly
max_steps points in every one of them (195/195, 594/594, 20/20, 12/12).
Measured against a live platform, the cost that had gone unaccounted: the
report is a synchronous call made from the backend's logging hook, on the
training thread, between one optimizer step and the next, and its latency
grows with the blob. 600 unthrottled reports spent 52 seconds blocked inside
the training loop, the last fifty averaging 2.4x the latency of the first
fifty. Same run through this gate: 14 seconds, and flat.
Move the cadence into the callback's one funnel, _report_metrics:
- Gate on elapsed steps, never a modulus. unsloth's on_log is already gated
by HuggingFace at logging_steps, and composing two moduli yields their LCM
rather than the finer of the two -- at logging_steps=3 against a target
interval of 100, a modulus draws a third of the points asked for. The
default of 1 divides everything, so it passes every test one would write
by default and then mangles the curve for anyone who touches the knob.
- Key the validation gate on the distinct step. validate() logs once per
dataloader at a single step, so the report counter this replaces could
admit one dataset and hold its neighbour, and burned the budget N times
faster with N dataloaders.
- Seed the interval from the run length and back it with double-and-decimate,
so the guarantee stops depending on an input we do not control.
- Rebuild the gate from the seeded series, so a resumed process continues the
cadence it inherited rather than restarting at full resolution. Read the
interval off the curve's average spacing, not its last gap: every run ends
by flushing a withheld step, which lands hard against its predecessor and
reads as an interval of one.
- Flush the withheld tail on close, without decimating it. A run whose length
divides evenly lands exactly max_points admissions and then flushes one
more; decimating there halved the finished curve, storing 101 points for
the 200 reported. Both found by measurement, not review.
NeMo-RL loses its copy: _MAX_REPORTS_PER_RUN, resolve_log_interval,
resolve_val_report_interval, _val_passes, _pending, _send and _flush_pending
all go, and NemoRLLogger returns to being an adapter. Its train path is
unchanged by construction; its validation path picks up the multi-dataloader
fix. automodel and unsloth gain bounded reporting and a final-point flush,
neither of which they had.
The throttle and flush coverage moves with the implementation, from
test_nemo_rl_logger.py to test_callbacks.py, where it now holds for all three
backends rather than one.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* feat(customization): expose the reporting budget in every backend's job config
The gate that bounds progress reporting landed with its budget fixed at 200.
Make it configurable, under one name and one shape across all three backends.
`ProgressReportingConfig` is deliberately not a NamespacedModel. That base
exists to keep two backends' same-named-but-different models from colliding in
the merged /apis/customization spec, and this is the opposite case: one model,
shared on purpose. The generated spec now carries a single
ProgressReportingConfig component referenced by AutomodelScheduleSpec,
RlDPOTraining and UnslothScheduleSpec, so the docs, the SDK and Studio see one
knob rather than three spellings of it.
It lives in its own module rather than beside the callback that consumes it so
the API layer can import the schema without pulling in the platform SDK that
training.progress needs.
Wiring, per backend:
- unsloth: ScheduleSpec, which the plugin re-exports, so input and output are
covered at once. train_sft reads it off the spec.
- rl: _TrainingBase -> TrainingStepConfig.ScheduleConfig -> the compiled DPO
block -> the driver -> NemoRLLogger. It rides the DPO block as an undeclared
extra alongside steps_per_epoch, that dict being the only channel from the
compiled job to the driver, and is read back with getattr because a config
compiled before this existed simply omits it.
- automodel: the longest chain -- plugin ScheduleSpec -> adapter -> the legacy
flat CustomizationJobOutput.training -> TrainingStepConfig.ScheduleConfig ->
the recipe config -> the callback. It reaches the training process as
`_progress_reporting`, underscore-prefixed like the existing
`_resolved_chat_template`, since the recipe config file is the only channel.
Every link defaults, which is what makes a broken one dangerous: the run would
report at 200 rather than fail, and nothing would say so. Each hop is therefore
pinned by a test, including compile_dpo_config -- previously untested because it
needs a real on-disk dataset, now exercised with a small one because no other
layer can see that hop.
Reading it back is defensive on the automodel side for a reason the others do
not share: the recipe config is also loadable from a hand-written YAML, and
_resolve_max_points runs in the wrapper's constructor, outside any try. A
missing, malformed or absurd value there costs the default, not the run.
Two related cleanups fall out. automodel's max_steps loses its `or 100`
fallback, which was unreachable -- StepScheduler always assigns max_steps and
asserts it positive, and the sentinel branch in _calculate_max_steps is dead
behind an assert. And unsloth's logging_steps gains the comment explaining why
it is not this knob: it drives stdout and W&B, and a user who wants verbose HF
logs should not trade the training curve for them.
The generated spec is regenerated here. The Stainless SDK and the CLI it
generates are not: `make stainless` needs STAINLESS_API_KEY, which is not set
in this environment, so the field reaches the Python SDK and `nemo` CLI on the
next sync.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* feat(customization): let a job choose which metrics get a stored curve
The stored blob is `curves x max_points`. `max_points` bounded one factor; this
bounds the other, which on most backends is the larger of the two. Measured
against the real metric dicts: NeMo-RL DPO accumulates 20 series, automodel 12,
unsloth 4, and Studio charts two of them.
Shipped as a knob defaulting to everything, with no per-backend defaults set.
Nobody's curves change until they ask, and what a good default set is stays a
product question rather than one settled in passing here.
The mechanism has to survive the objection that sank the last allow-list: one
gating what got *reported* dropped DPO's `accuracy`, `sft_loss` and
`rewards_chosen_mean` for a release because nobody had added them to it. This
one gates what gets *accumulated*. Every chartable metric is still reported as
a current value on every admitted report, so excluding one costs its history,
never its visibility — and the excluded names are logged, once per name rather
than once per step, with the set that is being charted beside them.
Names are matched unqualified, so `loss` covers both the training and
validation curves. One consequence is documented rather than solved: NeMo-RL's
logger folds the dataloader name into the metric names of every validation set
past the first, so a second set's `loss` arrives as `<dataset>_loss` and needs
that spelling to be charted. Only the first set — the only one any config we
compile produces — keeps the bare names.
Two shapes that must not collapse into each other, and are tested for it:
`None` charts everything and `[]` charts nothing. That distinction is why the
automodel reader treats an absent block and an explicit null as the same thing
without warning, while discarding a malformed list whole and saying so. A bare
string is the trap worth naming there: `curves: loss` in YAML is iterable, and
taken as a list it would chart the metrics `l`, `o` and `s`.
Plumbed the same three paths as max_points, and pinned at each hop for the same
reason: every link defaults, so a broken one silently charts everything rather
than failing.
The generated spec is regenerated. The Stainless SDK and CLI are deferred, as
with max_points.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(automodel)!: delete the inert log_every_n_steps
It described itself as "Logging frequency in steps. Controls how often training
metrics are logged" and controlled nothing. No code read it, it never reached
the recipe config, and `config.py` builds step_scheduler with no logging cadence
at all. A repo-wide grep found only the declaration.
The design doc called it "documented to users" and weighed deletion against
wiring it to StepScheduler.log_remote_every_steps, which gates W&B and MLflow
from inside log_train_metrics and so would have left our wrapper firing every
step. That wiring had a real argument behind it — it is the automodel analogue
of unsloth's logging_steps, which we deliberately keep separate from our own
reporting knob.
Checking the reachability settled it the other way. `api/v2/jobs/` holds a
schema file and no routes; CustomizationJobOutput is an internal intermediate
between the plugin adapter and the compiler; the submitter-facing
AutomodelJobOutput never carried the field; and regenerating the specs after
removing it produces no diff, because it was in none of them. No request could
ever set it. So there is no existing behaviour to preserve and no user
expectation to honour, and wiring it would have been adding a feature under
cover of a cleanup — a second field that sounds like "how often are metrics
logged", sitting next to progress_reporting.max_points, which is the knob it
was reaching for and which now exists.
Removal is invisible: the model ignores extras, so a stored spec that still
carries the key parses exactly as before. That property is the whole reason
this is safe rather than breaking, so it is pinned by a test — adding
extra="forbid" here later would otherwise turn silent tolerance into a hard
failure for precisely those specs.
Marked as a breaking change by convention, since the field was public in shape
even though it was unreachable in practice.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* test(automodel): pin that the reporting block written is the one read
The recipe config file is the only channel from a compiled automodel job to the
training process, and `_progress_reporting` is our key rather than the recipe's,
so nothing upstream validates it. Both sides were covered separately — the
compiler was not tested at all for this, and test_finetune.py exercises the
reader against a hand-built block — which is exactly the arrangement where a
rename passes every test and the run quietly reports at the default forever.
Every link in this chain defaults, so a break is silent by construction. This is
the test that fails instead.
Asserts exact equality rather than a subset: the reader's tests are written
against this literal shape, so a key renamed or dropped on either side now
breaks one of the two. Verified by mutation — renaming the written key fails it.
The equivalent hop in the other two backends stays untested, and cannot be
reached without the training images: unsloth's train_sft reads the spec behind
`import unsloth`, and NeMo-RL's driver reads the DPO block behind Ray. The
writer side of the NeMo-RL hop is covered by test_dpo_config.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(customization)!: name the time-series metrics, qualified and globbable
`curves` described the outcome and buried the thing that matters: a training run
produces two kinds of metric, and only one of them wants a history. A few — the
loss, the learning rate, the gradient norm — are read for their shape over time.
The rest are throughput and accounting counters whose current value is all
anyone looks at. Both kinds are always reported; only the first is accumulated.
`time_series_metrics` says which is which.
Names are now fully qualified, matching what appears in status_details, so what
a user writes is what they read back rather than something the callback prefixes
for them. That also deletes a caveat this design had been carrying: NeMo-RL
folds the dataloader name into the metric names of every validation set past the
first, so a second set's loss arrives as `val_heldout_loss`, which no
unqualified spelling could reach. It was documented as unfixable in
_curve_subset and in the design doc; both go.
Qualification doubles the list, so entries are globs, matched with fnmatchcase.
Not fnmatch, which applies os.path.normcase and would therefore match
case-insensitively on some platforms and not others — a genuinely horrible bug
to chase. `*_loss` covers both phases and the whole family (train_sft_loss,
val_preference_loss) and reaches the dataset-qualified names too; `*` records
everything. There is precedent on both counts: studio/config.py matches
allowed_origins the same way, and this API already takes `["*proj"]` for LoRA
target modules.
None now means the backend's default rather than everything, so `["*"]` is how
a user opts out of one. An empty list still means no series at all, and every
reader checks `is None` rather than truthiness so that stays distinguishable.
Deliberately one list, not two. The set of metric names comes from the framework
at runtime, so any pair of lists leaves a third category in neither, and the
rule for that category decides whether the second list does any work: unlisted →
scalar makes it documentation, unlisted → dropped is the failure that lost DPO's
metrics for a release, unlisted → error breaks every job on a framework upgrade.
Naming the series and letting the rest be scalars is the only partition that
survives a framework adding a metric. There is no cost argument for the other
shapes either: all 20 of NeMo-RL's scalars are ~400 bytes against a 172 KB blob.
Defaults are now set per backend, reversing the earlier decision to defer them.
Verified against the measured name sets: NeMo-RL 20 series to 14, automodel 12
to 5, unsloth 4 to 4 — and in every case only counters are dropped
(num_valid_samples, global_valid_*, mem, tps, tps_per_gpu, num_*_tokens). The
1.4x/2.4x/1x is the corrected arithmetic, not the doc's old 10x headline.
Qualification introduces one new way to be wrong: `loss` now matches nothing,
because the metric is `train_loss`, and the run then looks exactly like one
configured to record no history. close() warns about any pattern that never
matched, naming the metrics that did arrive. Left to close() because a name that
has not arrived yet is not yet wrong — validation metrics do not exist until the
first pass.
Free to reshape now only because the Stainless sync is deferred: the field is in
the API and the generated spec but in no SDK or CLI, so this is a rename with no
migration. After that sync it would need a deprecation.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): correct four data-loss defects in the progress gate
Found by an independent review pass, not by the tests — all 523 passed both
before and after, which is its own finding. Each fix below has a test that
fails on the preceding implementation; verified by stashing the source and
re-running.
**A second validation dataset overwrote the first's series.** Both LLM backends
validate over a dict of dataloaders and log once per entry at a single step.
NeMo-RL folds the dataset name into the metric names; automodel's wrapper
discarded the `val_name` its recipe passes, so every dataset reported as
`val_loss` — two datasets interleaved as two points at one step in one series,
which is the exact collision the `<phase>_` rule exists to prevent. Studio keys
its loss chart by step, so one silently won and the chart showed whichever
dataloader iterated last. The qualifier is now shared (`DatasetQualifier`)
rather than implemented for one backend and silently absent in the other.
**The final validation pass of all but one dataloader was dropped.** `pending`
was one slot per path, on the rationale that "a newer held report supersedes an
older one". True across steps, false for two dataloaders at one step: those are
different curves, not successive versions. When the last pass was withheld,
`close()` flushed whichever iterated last. `pending` is now keyed by the set of
series a report writes — which is precisely what distinguishes supersedes from
accompanies — and `_flush_pending` replays all of them. The dedup guard moved
with it: it compared the step against the path's last admission, which is true
for the second dataloader at a step the first just recorded, so it dropped the
very reports this fixes. It now asks whether *these* curves already hold the
point.
**Decimation coupled the future cadence to the size of the inherited past.**
`_decimate` halved once per call and raised the interval once per call, so a
curve k halvings over budget cost 2^k. Resuming a task whose series was written
by the uncapped callback that ships today (one point per step) took the interval
from a configured 100 to 6,400 and reduced the entire second half of a
20,000-step run to two reported points — reachable by any mixed-version resume
during the rollout of this very change. It now thins to the budget in one pass,
choosing the stride from the overshoot.
**And it compounded across dataloaders.** Because the raise was per call and each
dataloader's report is its own call, two datasets took a 25-step validation
cadence to 100 instead of 50, three would have taken it to 200. The interval is
now read off the thinned curve rather than multiplied, which removes both
compoundings at once: the stored points are the record of the cadence in force,
so deriving from them cannot double-count.
`_observed_spacing` (shared by `_seed_gate` and `_decimate`) rounds up rather
than down. Flooring returns one less than the interval actually in force for any
curve whose last point is a flush, and where this number is load-bearing — a
thinned curve — one step too small is what used to compound.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* refactor(customization)!: stop throttling, and let the transport own its cost
Removes `max_points` and the whole point-capping apparatus: the admission gate,
the elapsed-step interval, decimation and thinning, the spacing reconstruction,
the withheld-report machinery and the close-time flush. The callback now records
and sends whatever the training library hands it, at whatever cadence the
library logs. callbacks.py goes from 982 lines to 577.
The cap was solving a problem we do not have, in a place that should not have to
solve it.
We do not have it: every real customization job on record ran between 12 and 594
steps, and every Automodel contract fixture caps at 50. At that scale a
200-point budget never binds, and where it did bind it thinned a curve the user
wanted whole. The measured cost of not capping, at the scale runs actually
reach, is a fraction of a megabyte and seconds of reporting.
The place is wrong because the quadratic is not ours. It is entirely a property
of `status_details` being a blob that is replaced wholesale and written three
times — client to Jobs, Jobs to the task entity, Jobs to the job attempt. The
module docstring keeps that cost model, because it is still true and still needs
fixing; capping points here only made it cheaper to leave alone. The curves want
to live somewhere that appends.
And the cap was where the bugs were. Four data-loss defects were found in that
machinery by review last week — two dropping a dataloader's final validation
point, two letting decimation compound until a resumed run reported twice in
10,000 steps. None were in accumulation or in reporting. Deleting the mechanism
retires that entire class of defect rather than fixing it one instance at a time.
Verified against a live platform before removing the ceiling: a 20,000-step run
at 14 series is an 11.9 MB `status_details` blob and writes in ~2s. Nothing in
the path caps it — `data` is a JSON column, no body-size limit is configured
anywhere in the repo, and ingress is disabled by default and not on the write
path, since training pods report to the Jobs service in-cluster. Tested to 30 MB
without a failure or a truncation.
`time_series_metrics` stays. It is not a throttle: it says which metrics are
worth a history and which are worth only a current value, which is a
data-modelling choice about what a run means rather than a workaround for what
the transport costs. `DatasetQualifier` stays for the same reason — it fixes a
real collision between two validation datasets.
What a long run now costs is visible in the job rather than hidden behind a cap,
which is the right place for the pressure to sit.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(customization): stop a misconfigured metric list from killing the run
Three defects in `time_series_metrics`, all found by review, all on the path a
config file reaches. The NeMo-RL backend forwards this value straight out of a
YAML with no schema in between, and matching runs inline in a training step, so
"unusable" has to mean a warning rather than an exception.
**A non-string entry ended the run.** `fnmatchcase(name, None)` raises
`TypeError: object of type 'NoneType' has no len()`, and it raises from inside
`report_train_step`, which backends call from their logging hook with nothing
catching underneath. A stray `null` in a list cost the whole training job. The
list is now cleaned once at construction; unusable entries are dropped and
named in the log.
**A bare string silently recorded nothing.** `str` satisfies `Collection[str]`
as a collection of its own characters, so `time_series_metrics: train_loss` in
YAML became the ten patterns `t`, `r`, `a`, ... None of them match any metric,
so the run stored no history at all and said nothing about why. There is only
one thing that spelling can have meant, and it is now read that way.
A list whose every entry was unusable now falls back to recording everything,
on the principle that a broken config should cost noise rather than data. An
explicitly empty list is untouched -- `[]` is a legitimate request for current
values with no history, and it is the one input that rule must not swallow.
**The typo warning could not fire in the case it was written for.** It was gated
on `self._excluded_seen`, which holds metrics that were *dropped*, as a proxy
for "did any metric arrive". Those differ exactly when every metric that arrived
matched something -- the ordinary case -- so `["*_loss", "val_accuarcy"]`
against a run reporting only a loss excluded nothing, and the misspelling passed
in silence. It now tracks metrics seen, and names them in the warning so the
correct spelling is obvious.
Separately, `ProgressReportingConfig` now forbids extra fields. Every model it
is embedded in inherits that from `NamespacedModel`, and unsloth's schema module
states the contract outright -- "typos in the JSON shape become validation
errors, not silently-ignored fields". This fragment is deliberately not a
NamespacedModel, so that it emits one shared OpenAPI component rather than
three, and it did not inherit the strictness either: it was the one object in
the request body where `time_series_metric` parsed and then did nothing.
Six of the seven new tests fail on the preceding implementation, verified by
stashing the source. The seventh guards the empty-list case against the new
fallback and passes either way by design.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* feat(customization): rate-limit progress reports, keeping every point
Buffers metric reports in memory and decides when to send, rather than sending
one request per step the training library logs. Every point is still recorded at
full resolution the moment it arrives; only the cadence of the requests changes.
Measured on a 600-step burst: 600 requests become 2, and all 600 points are
still on the curve.
This is not the point cap in another guise, and the difference is the whole
reason it is safe. That gated the *record* -- points were discarded to stay
inside a budget, which is why it needed decimation, interval reconstruction from
the stored series, and a withheld-report slot per validation dataloader, and why
four data-loss defects lived in it. This gates only the *send*. A withheld
report loses nothing, because its points are already in the accumulator and
travel with the next request that goes. Multi-dataloader validation stops being
a special case entirely: there is no held payload whose identity has to be
tracked, just an accumulator that is sent less often.
What it is worth is bounded, and the docstring says so rather than overselling
it. On the runs that actually exist -- 12 to 594 steps -- it takes a 594-step
run from 594 requests and 36s blocked inside the training loop to 180 and 11s.
It does *not* bound a long run: the cost of one request grows with the
accumulated blob, so an 11-hour run still spends tens of minutes reporting at
any interval, and no rate limit can fix that because the payload is what grows.
That ceiling is the transport's, and remains the argument for getting the curves
out of `status_details`.
Details worth keeping:
- `time.monotonic`, not wall-clock time. A training process outlives NTP
corrections, and a clock stepping backwards would stall reporting until it
caught up. The clock is injectable, so the limiter is tested without sleeping.
- The first report always goes, so a curve starts at the beginning of a run and
the progress bar moves as soon as there is anything to say.
- The payload is built at send time, not at record time, so a withheld report
costs a dict of scalars instead of a copy of every series.
- Whether to attach `metrics` keys on anything unsent since the last send, not
on what this particular report added. Under a rate limit those differ: the
report that passes the limiter may itself be all current-value-only while
several withheld before it added points, and asking about only this one would
strand them.
- `close()` sends what the limiter withheld. Load-bearing for data rather than
freshness -- points recorded since the last send exist nowhere but this
process until something carries them.
- Checkpoint, epoch-end and training-start reports bypass the limiter. They are
events, not samples, and a checkpoint report carries the only record of where
the checkpoint landed.
- One interval across both paths, because the cost being limited is a request
and both paths send the same blob.
- A negative interval is clamped rather than rejected: this arrives from a job
config, and a reporting knob must not be able to stop a run from starting.
Exposed as `progress_reporting.min_report_interval_seconds`, defaulting to 10s,
plumbed through all three backends beside `time_series_metrics` and pinned at
each hop. The shared test helper disables it by default -- almost every test in
that file is about what a report contains, and those report several steps in one
instant.
Signed-off-by: Albert Cui <albcui@nvidia.com>
* fix(automodel,studio): switch two guards back on
Both were found by an independent review pass. Neither was failing; both had
stopped doing their job and said nothing, which is what makes them worth a
commit of their own.
**The contract check had disabled itself.** `compile_automodel_config` writes a
`_progress_reporting` block into every config, none of the fifteen golden
fixtures have it, so every config mismatched. The test inferred its embedding-
config exclusion from the *output* of the check -- any run whose failure text
mentioned an excluded stem skipped the whole thing -- so a change touching every
config guaranteed the text mentioned one, and the check silently stopped running
with no failure and no skip message anyone would look at twice.
Two fixes, because the block was …1 parent d6fc211 commit b090c98
43 files changed
Lines changed: 4909 additions & 378 deletions
File tree
- docs/customizer
- manage-customization-jobs
- tutorials
- packages/nmp_customization_common
- src/nmp/customization_common/training
- tests/training
- plugins
- nemo-automodel/src/nemo_automodel_plugin
- nemo-customizer/openapi
- services
- automodel
- src/nmp/automodel
- api/v2/jobs
- app/jobs/training
- tasks/training/backends
- tests
- contract
- tasks/training/backends
- rl
- src/nmp/rl
- app/jobs
- training
- tasks/training
- backends/nemo_rl
- tests
- unsloth
- src/nmp/unsloth
- tasks/training/backends
- tests
- web/packages/studio/src
- mocks/customizer
- util
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 45 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
17 | 19 | | |
18 | 20 | | |
| 21 | + | |
| 22 | + | |
19 | 23 | | |
20 | 24 | | |
21 | 25 | | |
| |||
149 | 153 | | |
150 | 154 | | |
151 | 155 | | |
| 156 | + | |
152 | 157 | | |
153 | | - | |
154 | | - | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
155 | 175 | | |
156 | 176 | | |
157 | 177 | | |
| |||
225 | 245 | | |
226 | 246 | | |
227 | 247 | | |
| 248 | + | |
228 | 249 | | |
229 | | - | |
230 | | - | |
| 250 | + | |
| 251 | + | |
231 | 252 | | |
232 | | - | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
233 | 272 | | |
234 | 273 | | |
235 | 274 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | | - | |
| 33 | + | |
| 34 | + | |
34 | 35 | | |
35 | | - | |
36 | | - | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
37 | 122 | | |
38 | 123 | | |
39 | 124 | | |
| |||
67 | 152 | | |
68 | 153 | | |
69 | 154 | | |
70 | | - | |
71 | | - | |
| 155 | + | |
| 156 | + | |
72 | 157 | | |
73 | 158 | | |
74 | | - | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
75 | 169 | | |
76 | 170 | | |
77 | 171 | | |
| |||
0 commit comments