Skip to content

Commit 5d2df92

Browse files
committed
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>
1 parent 752ea56 commit 5d2df92

2 files changed

Lines changed: 36 additions & 4 deletions

File tree

services/automodel/src/nmp/automodel/api/v2/jobs/schemas.py

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -211,14 +211,17 @@ class _TrainingBase(BaseModel):
211211
gt=0,
212212
description="Max training steps. Overrides epochs if set.",
213213
)
214-
log_every_n_steps: Optional[int] = Field(
215-
default=None,
216-
description="Logging frequency in steps. Controls how often training metrics are logged.",
217-
)
218214
val_check_interval: Optional[float] = Field(
219215
default=None,
220216
description="Validation interval. Float <= 1.0 is fraction of epoch; > 1.0 is step count.",
221217
)
218+
# `log_every_n_steps` used to sit here, described as "Logging frequency in steps.
219+
# Controls how often training metrics are logged." It controlled nothing: no
220+
# code read it, it never reached the recipe config, and it was absent from the
221+
# submitter-facing plugin schema and from every generated spec, so no request
222+
# could set it in the first place. `progress_reporting` below is the knob it
223+
# was reaching for. Removing it is invisible -- this model ignores extras, so a
224+
# stored spec that still carries the key parses exactly as it did before.
222225
progress_reporting: ProgressReportingConfig = Field(default_factory=ProgressReportingConfig)
223226

224227
# --- Batch ---

services/automodel/tests/test_compiler.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,35 @@ def test_the_reporting_budget_reaches_the_training_step_config() -> None:
126126
assert cfg["schedule"]["progress_reporting"]["curves"] == ["loss"]
127127

128128

129+
def test_a_spec_still_carrying_log_every_n_steps_compiles() -> None:
130+
"""The removed field was inert, and removing it has to stay invisible.
131+
132+
It described itself as controlling how often training metrics are logged and
133+
controlled nothing: nothing read it, it never reached the recipe config, and
134+
it was in neither the submitter-facing plugin schema nor any generated spec.
135+
What makes deleting it safe rather than breaking is that this model ignores
136+
extras -- so a stored spec that still carries the key parses as it always
137+
did. Pinned because a later `extra="forbid"` here would turn that silent
138+
tolerance into a hard failure for exactly those specs.
139+
"""
140+
from nmp.automodel.app.jobs.training.compiler import compile_training_step
141+
from nmp.customization_common.training.reporting import DEFAULT_MAX_POINTS
142+
143+
training = SFTTraining.model_validate({"learning_rate": 1e-4, "log_every_n_steps": 10})
144+
assert not hasattr(training, "log_every_n_steps")
145+
146+
job_output = CustomizationJobOutput(
147+
model="default/test-target",
148+
dataset="default/my-dataset",
149+
training=training,
150+
output=OutputResponse(name="out", type="adapter", fileset="out-fs"),
151+
)
152+
step = compile_training_step(job_output, base_env=[], me=_make_mock_model_entity())
153+
cfg = step.config if hasattr(step, "config") else step["config"]
154+
155+
assert cfg["schedule"]["progress_reporting"]["max_points"] == DEFAULT_MAX_POINTS
156+
157+
129158
def test_the_reporting_budget_survives_the_plugin_adapter() -> None:
130159
"""The adapter flattens the plugin's schedule block and is easy to drop a field from."""
131160
from nmp.automodel.adapter import automodel_spec_to_compiler_output

0 commit comments

Comments
 (0)