Skip to content

fix(launcher): reject nemo_run overrides the executor has no attribute for - #3949

Open
kabirvashisht4-glitch wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
kabirvashisht4-glitch:kabirvashisht4-glitch/fix/reject-unknown-executor-overrides
Open

kabirvashisht4-glitch wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
kabirvashisht4-glitch:kabirvashisht4-glitch/fix/reject-unknown-executor-overrides

Conversation

@kabirvashisht4-glitch

Copy link
Copy Markdown
Contributor

What does this PR do ?

Fixes #3948: a key in the nemo_run: YAML section that the executor has no field for was
silently absorbed instead of reported.

NemoRunConfig.from_dict collects every unrecognised key into overrides, and
apply_overrides assigned each with a bare setattr. NeMo-Run executors are dataclasses
with fixed fields, so assigning an unknown name does not fail — it adds an attribute
nothing reads, while the field the user meant to set keeps its default:

ex = FakeSlurmExecutor()   # nodes=1, ntasks_per_node=1, partition="batch"
apply_overrides(ex, {"ntasks_per_nodes": 8, "paritition": "gpu"})

ex.ntasks_per_node   # 1       <- user asked for 8
ex.partition         # "batch" <- user asked for "gpu"

The job is then submitted with one task per node, or on the wrong partition, with no
exception, warning, or log line. The symptom appears much later as a mis-sized job, and
nothing points back at the YAML.

Approach

Check hasattr before assigning and raise a ValueError naming the executor and its
available attributes. This follows two precedents rather than inventing a convention:

  • load_executor_from_file already raises for an unknown executor name with
    "... Available: alpha, beta, gamma"; the new message mirrors it.
  • _configure_torchrun, in the same class that calls apply_overrides, already guards
    the identical operation with if hasattr(executor, "torchrun_nproc_per_node").
    apply_overrides was the outlier.

.agents/contributor-skills/testing also prescribes exactly this guard for foreign
setattr, including the ValueError.

This is a deliberate behavior change

A config that today silently drops a key will now fail at submission. I want to be
explicit about that rather than bury it: those configs are already not doing what they
say, and failing at submit time is far cheaper than discovering it after a multi-node job
has run with the wrong topology. If you would rather this warn than raise for a release
or two, say so and I will switch it to logger.warning — the guard and tests are the
same either way.

Why the existing tests could not catch it

Every apply_overrides test passes a mock.MagicMock(), which fabricates any attribute
on access, so the suite cannot distinguish setting a real field from inventing one. No
existing test asserts that an unknown key is accepted, so the permissive behavior was
untested rather than intended. The new tests use a dataclass stand-in, which is what the
real executors are.

Changelog

  • apply_overrides raises ValueError when the executor has no attribute for an
    override key, naming the executor type and listing its available attributes.
  • Document the new Raises, plus Args, on apply_overrides.
  • Add TestApplyOverridesRejectsUnknownAttributes against a dataclass executor
    stand-in: an unknown key raises and creates no phantom attribute; the message names the
    executor and its fields; known scalar/dict/list overrides still apply on a real object;
    and an unknown key later in the mapping does not silently pass.

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?

Verified locally:

  • pytest tests/unit_tests/launcher/ -> 104 passed (100 on clean main; the delta is
    exactly the 4 tests added here). No existing test changed behavior — the MagicMock
    ones still pass, since hasattr is always true on a mock.
  • 3 of the 4 new tests fail on main. Reverting only the nemo_automodel/ change and
    keeping the tests reproduces those 3. The 4th,
    test_known_keys_still_apply_on_a_real_object, passes on main by design — it is the
    guard-rail that the accepted path is unaffected, not a regression test.
  • ruff format --check and ruff check clean on both changed files. The test file is
    not ruff-formatted on main; I deliberately left that pre-existing churn alone, so the
    test diff is additions only.
  • CPU only. nemo_run is not installed and is not needed — the tests use a dataclass
    stand-in, matching how the rest of this file stubs nemo_run.

Additional Information

  • Both production call sites (_resolve_executor, for the "local" executor and for a
    named one from EXECUTOR_MAP) go through the guarded path.

…e for

`NemoRunConfig.from_dict` collects every unrecognised key of the `nemo_run:`
YAML section into `overrides`, and `apply_overrides` assigned each one with a
bare `setattr`. NeMo-Run executors are dataclasses with fixed fields, so a
mistyped key did not fail -- it created an attribute nothing reads and left the
field the user meant to set at its default.

`ntasks_per_nodes: 8` leaves `ntasks_per_node` at 1 and the job runs one task
per node; `paritition: gpu` leaves the job on the default partition. Neither
raises, warns, or logs.

Check `hasattr` before assigning and raise a ValueError naming the executor and
its available attributes, matching `load_executor_from_file`'s "Available: ..."
error and the `hasattr` guard `_configure_torchrun` already uses next door.

The existing tests drive `apply_overrides` with a MagicMock, which fabricates
any attribute on access and so cannot tell a real field from one the executor
does not have; the new tests use a dataclass stand-in instead.

Signed-off-by: kabirvashisht4-glitch <kabirvashisht4@gmail.com>
@kabirvashisht4-glitch
kabirvashisht4-glitch requested a review from a team as a code owner September 20, 2026 10:49
@copy-pr-bot

copy-pr-bot Bot commented Sep 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nemo_run executor overrides silently ignore keys the executor has no field for

1 participant