fix(core): match converter source types without raw isinstance by CodeAlex52 · Pull Request #2310 · NVIDIA/NeMo-Agent-Toolkit · GitHub
Skip to content

fix(core): match converter source types without raw isinstance - #2310

Merged
rapids-bot[bot] merged 1 commit into
NVIDIA:developfrom
CodeAlex52:fix/type-converter-safe-instance-checks
Oct 15, 2026
Merged

rapids-bot[bot] merged 1 commit into
NVIDIA:developfrom
CodeAlex52:fix/type-converter-safe-instance-checks

Conversation

@CodeAlex52

@CodeAlex52 CodeAlex52 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

A converter registered from a source annotation that isinstance rejects could not be used. The
converter compared the registered source type against the incoming data with a raw isinstance:

decomposed_from = DecomposedType(convert_from_type)
check_type = convert_from_type if decomposed_from.is_union else decomposed_from.root
if isinstance(data, check_type):

isinstance rejects several valid annotations:

registered source result
typing.Any TypeError: typing.Any cannot be used with isinstance()
typing.Literal["a"] TypeError: typing.Literal cannot be used with isinstance()
dict[str, int] | None (union with a parameterized generic) TypeError: isinstance() argument 2 cannot be a parameterized generic
str | None works — the reason the union branch exists

The same applies to the target side (TypeConverter._convert and is_instance) and to the
indirect path, which had its own union special case.

Fix

  • Both matching paths now use DecomposedType.is_instance, the safe check this file already
    documents for parameterized generics ("Use DecomposedType for safe isinstance check with
    parameterized generics"), and the one the target side already uses. The ad-hoc union branches
    are removed; a union is resolved member by member inside is_instance.
  • DecomposedType.is_instance is made safe for the same constructs:
    • Literal[...] constrains the value, not the class, so it is matched against its literal
      arguments with a type check (True does not satisfy Literal[1], and vice versa).
    • Any other construct whose root is not a class matches nothing instead of raising.

Tests

  • test_converter.py: sources registered from Any, Literal["a"] (match and no-match), and
    dict[str, int] | None (both members, direct and through an indirect chain), plus targets of
    Any and Literal. All of these fail before the change.
  • test_type_utils.py: is_instance with Literal (including the bool / int distinction) and
    with constructs that keep a non-class root (NoReturn, Final[int]).

Verification: test_converter.py + test_type_utils.py → 93 passed (87 before); reverting the
source change fails exactly the 6 new tests. yapf --style ./pyproject.toml and ruff check
(versions pinned in .pre-commit-config.yaml) are clean.

Note: this branch is based on develop before #2309, which also touches type_utils.py
(is_subtype). The two changes are in different methods; I will rebase if #2309 lands first.

Summary by CodeRabbit

  • Bug Fixes
    • Improved converter matching for unions, generic types, Any, and Literal annotations.
    • Literal matching now requires both the value and its exact type to match, so values such as True do not match Literal[1].
    • Unsupported type constructs no longer cause matching errors.

The type converter compared a registered source annotation against the incoming
data with isinstance, which rejects valid annotations: typing.Any and
typing.Literal raise TypeError, and a union containing a parameterized generic
(dict[str, int] | None) raises 'isinstance() argument 2 cannot be a parameterized
generic'. Registering a converter from any of those made the conversion fail,
including conversions that did not reach that converter first.

Both the direct and the indirect matching path now use DecomposedType.is_instance,
the safe check this file already relies on for targets and parameterized generics.

is_instance is also made safe for the same constructs: Literal constrains the value
rather than the class, so it is matched against its arguments (type checked so True
does not satisfy Literal[1]), and any other non-class root matches nothing instead
of raising.

Signed-off-by: CodeAlex52 <59381946+CodeAlex52@users.noreply.github.com>
@CodeAlex52
CodeAlex52 requested a review from a team as a code owner October 10, 2026 08:04
@copy-pr-bot

copy-pr-bot Bot commented Oct 10, 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.

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/NeMo-Agent-Toolkit/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 3474fd60-bc2b-41e7-a537-bec2ed7f8101

📥 Commits

Reviewing files that changed from the base of the PR and between be97c3a and 40bd53a.


📒 Files selected for processing (4)
  • packages/nvidia_nat_core/src/nat/utils/type_converter.py
  • packages/nvidia_nat_core/src/nat/utils/type_utils.py
  • packages/nvidia_nat_core/tests/nat/utils/test_converter.py
  • packages/nvidia_nat_core/tests/nat/utils/test_type_utils.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.



Walkthrough

DecomposedType.is_instance now handles Literal values and returns False for non-class roots. Direct and indirect converter lookup use this helper to match registered input annotations. Tests cover Any, Literal, and unions containing parameterized generics.

Changes

Converter annotation matching

Layer / File(s) Summary
Type instance matching
packages/nvidia_nat_core/src/nat/utils/type_utils.py, packages/nvidia_nat_core/tests/nat/utils/test_type_utils.py
DecomposedType.is_instance matches Literal values only when both type and value match. It returns False for other non-class roots. Tests cover these cases, including True versus Literal[1].
Converter lookup integration
packages/nvidia_nat_core/src/nat/utils/type_converter.py, packages/nvidia_nat_core/tests/nat/utils/test_converter.py
Direct and indirect lookup use DecomposedType.is_instance to check registered input annotations. Tests cover source annotations using Any, Literal, and dict[str, int] | None, plus Any and Literal targets.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 40bd5

No merge-blocking issue was established; the change is ready for normal checks.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title is concise, descriptive, uses imperative mood, is 62 characters long, and accurately describes the converter source-type matching changes.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@willkill07 willkill07 added bug Something isn't working non-breaking Non-breaking change labels Oct 15, 2026
@willkill07

Copy link
Copy Markdown
Member

/ok to test 40bd53a

@willkill07

Copy link
Copy Markdown
Member

@rapids-bot
rapids-bot Bot merged commit 123a371 into NVIDIA:develop Oct 15, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants