fix(cli): resolve string and future annotations in cli parser (#374) by ManoharPaturi · Pull Request #613 · NVIDIA-NeMo/Run · GitHub
Skip to content

fix(cli): resolve string and future annotations in cli parser (#374) - #613

Merged
ko3n1g merged 8 commits into
NVIDIA-NeMo:mainfrom
ManoharPaturi:fix/cli-resolve-string-annotations-374
Oct 3, 2026
Merged

ko3n1g merged 8 commits into
NVIDIA-NeMo:mainfrom
ManoharPaturi:fix/cli-resolve-string-annotations-374

Conversation

@ManoharPaturi

Copy link
Copy Markdown
Contributor

Fixes #374.

Motivation

When recipes or functions are defined in modules with from __future__ import annotations (or use string annotations / ForwardRefs), parameter annotations are stored as string expressions (e.g. 'int', 'str', 'list[str]', 'int | None').

Previously, _maybe_resolve_annotation only queried _resolve_type_checking_annotation, which scanned if TYPE_CHECKING: blocks in AST. String annotations for built-in types (like 'int') or module globals were never resolved to type objects, causing TypeParser.parse to fall through to UnknownTypeError: Unsupported type: int.

Solution

  1. In nemo_run/cli/cli_parser.py:
    • Added _resolve_string_annotation which resolves string annotations by first checking builtins (e.g. int, str, float, bool), then evaluating in the namespace of builtins, typing, and the target function/class's module globals, and falling back to AST TYPE_CHECKING imports.
    • Updated _maybe_resolve_annotation to use _resolve_string_annotation for string and ForwardRef annotations, and support types.UnionType (PEP 604 X | Y).
    • Added a check in TypeParser.get_parser to inspect builtins when annotation is a string matching a built-in type name.
  2. Added unit tests in test/cli/test_cli_parser.py covering scalar string annotations, container string annotations, and PEP 604 unions.

All 82 tests in test/cli/test_cli_parser.py pass cleanly.

…-NeMo#374)

Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 09:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@copy-pr-bot

copy-pr-bot Bot commented Oct 2, 2026

Copy link
Copy Markdown

@ko3n1g

ko3n1g commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

/ok to test 6884b90

@ko3n1g

ko3n1g commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

/review

@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.

Please help with the linting issues, i'll get it merged afterwards. Thanks for your contribution!

@svcnvidia-nemo-ci svcnvidia-nemo-ci 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.

The added tests cover simple string annotations, but not a function that actually enables future annotations. The resolver stops after one evaluation, so valid quoted and nested forward references still fail. Please address the inline issue and add an actual future-annotations regression test. Static review only; tests were not run.

Review generated by codex

Comment thread nemo_run/cli/cli_parser.py Outdated
if isinstance(annotation, str):
resolved = _resolve_type_checking_annotation(fn, annotation)
return resolved if resolved != annotation else annotation
return _resolve_string_annotation(fn, annotation)

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.

[P2] Resolve forward references in the evaluated result

With from __future__ import annotations, from pathlib import Path, and def func(path: "Path"), the signature annotation is "'Path'". _resolve_string_annotation evaluates that once and returns the string "Path", which is passed straight to TypeParser; because its string fallback only recognizes builtins, parse_cli_args(func, ["path=/tmp/x"]) still raises UnknownTypeError. Similarly, evaluating a future annotation such as list["Path"] leaves its inner string unresolved. Continue resolving strings/ForwardRefs in the evaluated annotation, preserving Literal values, and add a regression test that actually enables the future import.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in e3ff804. _resolve_string_annotation now feeds whatever it evaluates back through _maybe_resolve_annotation with a depth cap, so a quoted "'Path'" under future annotations resolves through the extra string layer and list["Path"] resolves the inner ForwardRef. Literal args are skipped by the recursion so Literal["Path"] keeps its string values. added regressions using a module that actually enables from future import annotations (exec'd so the future import applies only to it): the quoted-name case from your report, nested forward refs in a container, Literal preservation, and a dataclass config. the first two fail on the single-evaluation version.

@ko3n1g

ko3n1g commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

/ok to test 67e3c24

@copy-pr-bot

copy-pr-bot Bot commented Oct 2, 2026

Copy link
Copy Markdown

/ok to test 67e3c24

@ko3n1g, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@ko3n1g

ko3n1g commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

/ok to test 6762f08

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Oct 2, 2026
Resolve the cli_parser conflict by keeping this branch's resolver
helpers (_get_type_checking_imports, _resolve_string_annotation) and
folding main's NVIDIA-NeMo#607 unparameterized-generics early return into
_maybe_resolve_annotation.

Also address the review finding on forward references: a quoted name
under future annotations ("'Path'") evaluates to another string, and
container annotations like list["Path"] evaluate to generics holding
unresolved ForwardRefs. _resolve_string_annotation now feeds its
evaluated result back through _maybe_resolve_annotation (depth capped)
instead of stopping after one evaluation, and Literal args are skipped
by the recursion so Literal["Path"] keeps its string values. Added
regressions for the real future-annotations module, nested forward
refs, Literal preservation, and class-based configs.
@ManoharPaturi

Copy link
Copy Markdown
Contributor Author

@ko3n1g done in e3ff804. the format failure is fixed (verified with the pinned ruff 0.6.1, both format and lint clean), the main conflict in cli_parser.py is resolved keeping #607's unparameterized-generics handling, and the full test/cli suite passes locally, 322 tests. codecov should also improve, the diff now has five more resolver tests.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Oct 3, 2026
_resolve_type_checking_annotation returns the original annotation string
on a miss, so assigning its result unconditionally made step 4's
compound-expression evaluation unreachable: Optional[Path] with Path
imported only under if TYPE_CHECKING: stayed a raw string and parsing
raised UnknownTypeError. Only a changed direct lookup now counts as
resolved.

Added a fixture module that enables future annotations with
TYPE_CHECKING-only imports, with regressions for the scalar and
container compound cases plus the unresolvable fallback.
@ManoharPaturi

Copy link
Copy Markdown
Contributor Author

@ko3n1g the codex finding is addressed in 403179a (the compound TYPE_CHECKING fallback was unreachable, now fixed with regressions). lint and format verified clean with the pinned ruff 0.6.1, full test/cli suite passes locally, 360 tests.

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-customer Waiting on the original author to respond label Oct 3, 2026
@ko3n1g

ko3n1g commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

/ok to test 403179a

@ko3n1g

ko3n1g commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

/review

@svcnvidia-nemo-ci svcnvidia-nemo-ci 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.

The resolver and added tests cover quoted future annotations, nested references, Literal preservation, and PEP 604 unions. One correctness gap remains: the compound TYPE_CHECKING fallback discards module globals before retrying, so valid annotations using runtime aliases still fail. Tests were inspected but not executed.

Review generated by codex

Comment thread nemo_run/cli/cli_parser.py Outdated
if resolved is None:
type_checking_imports = _get_type_checking_imports(fn)
if type_checking_imports:
ns = dict(builtins.__dict__)

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.

[P2] Preserve module globals in the TYPE_CHECKING retry

With future annotations enabled, from typing import Optional as Opt, and Path imported only under if TYPE_CHECKING:, a parameter annotated Opt[Path] fails the first evaluation because Path is unavailable. This fallback then replaces the namespace with builtins and typing names, losing Opt. Its evaluation also fails, leaving the raw annotation string, so parse_cli_args(func, ["path=/tmp/x"]) raises UnknownTypeError instead of producing a Path. Extend the namespace constructed in step 2 with the resolved TYPE_CHECKING imports rather than rebuilding it, and add a regression using a runtime alias.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed in 1352582. both evaluation passes now share one namespace (builtins + typing + module globals), and the retry layers the resolved TYPE_CHECKING names on top of it instead of rebuilding from scratch, so Opt survives while Path becomes available. regression added to the fixture module combining the runtime alias with the TYPE_CHECKING-only import; it fails on 403179a and passes on the new head.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-customer Waiting on the original author to respond label Oct 3, 2026
The compound-expression fallback rebuilt its evaluation namespace from
builtins and typing only, so runtime aliases from module globals
(from typing import Optional as Opt) were lost and Opt[Path] with a
TYPE_CHECKING-only Path stayed an unresolved string. Both evaluation
passes now share one namespace: builtins, typing, and module globals,
with the statically-imported names layered on top for the retry.

Regression uses a fixture module combining the alias with a
TYPE_CHECKING-only Path import.
@ManoharPaturi

Copy link
Copy Markdown
Contributor Author

@ko3n1g the follow-up codex finding is fixed in 1352582 (the TYPE_CHECKING retry kept dropping module globals like runtime aliases; one shared namespace now). full test/cli suite 361 passed locally, pinned ruff 0.6.1 format and lint clean.

@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-customer Waiting on the original author to respond label Oct 3, 2026
@ko3n1g

ko3n1g commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

/ok to test 1352582

@ko3n1g

ko3n1g commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

/review

@svcnvidia-nemo-ci svcnvidia-nemo-ci 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.

Verified the prior namespace finding is fixed: the TYPE_CHECKING retry now preserves module globals, including runtime aliases such as Opt. The added fixture and regression test cover this combination. The earlier compound-fallback and recursive-resolution fixes remain in place. No merge-blocking defects found in the reviewed changes and directly affected parsing paths.

The prior reviewed tree was not mounted, so an exact inter-review comparison was unavailable; follow-up inspection used the supplied history and current diff. Tests were inspected, not executed.

Review generated by codex

@ko3n1g
ko3n1g added this pull request to the merge queue Oct 3, 2026
@ko3n1g

ko3n1g commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Merged via the queue into NVIDIA-NeMo:main with commit 20bac13 Oct 3, 2026
29 of 31 checks passed

This branch was successfully deployed

1 active deployment
public — 13525828 Deployed Oct 3, 2026 by copy-pr-bot[bot] via release / finalize / publish-docs #373
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.

CLI parser throws an error when providing a config via yaml flag

4 participants