Skip to content

Fix -y short flag collision between --yaml and --yes (#559) - #561

Open
CodersAcademy006 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
CodersAcademy006:fix/cli-yaml-yes-short-flag-collision
Open

Fix -y short flag collision between --yaml and --yes (#559)#561
CodersAcademy006 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
CodersAcademy006:fix/cli-yaml-yes-short-flag-collision

Conversation

@CodersAcademy006

Copy link
Copy Markdown
Contributor

Summary

In RunContext.cli_command (nemo_run/cli/api.py), both --yaml and --yes/--no-confirm claimed the short flag -y, so Click emitted a warning on every invocation. This drops -y from --yaml and keeps it as the conventional alias for --yes.

Closes #559

cc @hemildesai @marcromeyn

@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

@ko3n1g @chtruong814 could one of you take a look at this and at #562? Both have been open since July 14 with no reviewer assigned, and the original cc to @hemildesai and @marcromeyn was not picked up.

This one is a single line in nemo_run/cli/api.py. In RunContext.cli_command both --yaml and --yes/--no-confirm claim the short flag -y, so Click emits a warning on every invocation. It drops -y from --yaml and leaves it as the conventional alias for --yes. Closes #559.

Copy link
Copy Markdown
Contributor

PR #561 — needs attention
Blocking findings: None.
Non-blocking finding: docs/guides/cli.md:349 still advertises -y for --yaml, while nemo_run/cli/api.py:893-900 now reserves -y for --yes. Update the generated/example option table so users are not given a removed alias.
Validation gaps: No regression test asserts warning-free help or verifies that -y selects confirmation bypass. Only DCO is visible.

@ko3n1g

ko3n1g commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@CodersAcademy006 could you take a look at #561 (comment) please?

@ko3n1g ko3n1g added the waiting-on-customer Waiting on the original author to respond label Aug 7, 2026
@CodersAcademy006

CodersAcademy006 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Hey @ko3n1g Fixed all three blockers. Removed -y from yaml option in api.py line 894, updated docs at 349, added TestShortFlagCollision regression tests to verify -y works for skip_confirmation and yaml has no short flag. Ready to merge.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-maintainers Waiting on maintainers to respond and removed waiting-on-customer Waiting on the original author to respond labels Aug 7, 2026
ko3n1g
ko3n1g previously approved these changes Aug 19, 2026

@ko3n1g ko3n1g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ko3n1g

ko3n1g commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

hey @CodersAcademy006, there's unfortunately another linting and testing issue left. Could you take a look?

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Aug 19, 2026
RunContext.cli_command bound -y to both --yaml and --yes/--no-confirm,
so Click emitted a warning on every invocation. Drop -y from --yaml,
keeping -y as the conventional alias for --yes, and update the option
table in docs/guides/cli.md to match.

Adds TestShortFlagCollision, which asserts the paired invariant from the
generated Click command: --yaml carries no short flag, --yes still owns
-y, and no two options share a short flag. It fails if -y is put back on
--yaml.

Closes NVIDIA-NeMo#559

Signed-off-by: Srijan Upadhyay <srjnupadhyay@gmail.com>
@CodersAcademy006

Copy link
Copy Markdown
Contributor Author

@ko3n1g fixed and force-pushed as b31d4fa. Three separate things were red: DCO, because my second commit was not signed off; ruff format, because dropping -y let the yaml option fit on one line and my docs table edit was two spaces out of column; and two of the three regression tests, which failed for a real reason. They invoked the command instead of inspecting it, so --yaml config.yaml made RunContext try to load a file that does not exist and -y executed the dummy task for real. Both exited 1 through no fault of the fix.

I squashed the branch into one signed commit and replaced those tests with a single TestShortFlagCollision that reads the generated Click command rather than running it: --yaml carries no short flag, skip_confirmation still owns -y, and no two options share a short flag. I checked it actually catches the regression by putting -y back on --yaml, where it fails. Locally test/cli is 310 passed, ruff format --diff and ruff check are clean, and the full suite matches the upstream main baseline exactly (25 failures either side, all from skypilot and kubeflow extras missing in my env). DCO is green now, but the nine Actions workflows are sitting at action_required and need your approval to start.

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-customer Waiting on the original author to respond label Aug 19, 2026
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.

RunContext.cli_command assigns the short flag -y to both --yaml and --yes — click warns on every invocation

4 participants