Repository navigation
Conversation
…-NeMo#374) Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
|
/ok to test 6884b90 |
|
/review |
ko3n1g
left a comment
There was a problem hiding this comment.
Please help with the linting issues, i'll get it merged afterwards. Thanks for your contribution!
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
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
| if isinstance(annotation, str): | ||
| resolved = _resolve_type_checking_annotation(fn, annotation) | ||
| return resolved if resolved != annotation else annotation | ||
| return _resolve_string_annotation(fn, annotation) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
|
/ok to test 67e3c24 |
@ko3n1g, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 6762f08 |
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.
|
@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. |
_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.
|
/ok to test 403179a |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
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
| if resolved is None: | ||
| type_checking_imports = _get_type_checking_imports(fn) | ||
| if type_checking_imports: | ||
| ns = dict(builtins.__dict__) |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
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.
|
/ok to test 1352582 |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
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

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_annotationonly queried_resolve_type_checking_annotation, which scannedif TYPE_CHECKING:blocks in AST. String annotations for built-in types (like'int') or module globals were never resolved to type objects, causingTypeParser.parseto fall through toUnknownTypeError: Unsupported type: int.Solution
nemo_run/cli/cli_parser.py:_resolve_string_annotationwhich resolves string annotations by first checkingbuiltins(e.g.int,str,float,bool), then evaluating in the namespace ofbuiltins,typing, and the target function/class's module globals, and falling back to ASTTYPE_CHECKINGimports._maybe_resolve_annotationto use_resolve_string_annotationfor string andForwardRefannotations, and supporttypes.UnionType(PEP 604X | Y).TypeParser.get_parserto inspectbuiltinswhenannotationis a string matching a built-in type name.test/cli/test_cli_parser.pycovering scalar string annotations, container string annotations, and PEP 604 unions.All 82 tests in
test/cli/test_cli_parser.pypass cleanly.