Class Template - Method Template (Parent-Child pattern) Binding Generation #286
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cialization inherit, but instead own an instantiation object
…overload selection helper
…lecting overloads
…lecting overloads
Contributor
📝 WalkthroughWalkthroughImplements per-method templated-method binding with arg_intent propagation, enum-aware rendering of integral template arguments, CCCL type mappings and new CCCL tests, CI wiring to run CCCL tests, and multiple API/shim-generation and test updates across ast_canopy and numbast. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Binder as Binding Layer (numbast)
participant Shim as ShimWriter / Shim Generator
participant NVRTC as CUDA Compiler
participant GPU as Device Runtime
Test->>Binder: request templated-method binding (with arg_intent)
Binder->>Binder: normalize arg_intent, resolve per-method overrides
Binder->>Binder: select templated overload(s) / create AbstractTemplate
Binder->>Shim: generate shim & kernel source for selected specialization
Shim->>NVRTC: compile CUDA module
NVRTC->>GPU: load/launch kernel
Test->>GPU: prepare buffers and invoke kernel via compiled module
GPU-->>Test: return results
Test->>Test: validate outputs (assert correctness)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ast_canopy/ast_canopy/instantiations.py (1)
60-68: Missing@propertydecorators on abstract methods.
base_name(line 60) andqual_name(line 65) are accessed as properties viaself.base_nameandself.qual_nameat lines 42-44, but they lack@propertydecorators. This causesself.base_nameto return the method object instead of raisingNotImplementedError.🐛 Proposed fix
+ `@property` def base_name(self): raise NotImplementedError( "BaseInstantiation.base_name is not implemented" ) + `@property` def qual_name(self): raise NotImplementedError( "BaseInstantiation.qual_name is not implemented" )
🤖 Fix all issues with AI agents
In @.github/workflows/wheels-test.yaml:
- Around line 136-138: The CI step named "Run a specific set of
numbast_extensions tests" is too vague—update the step's name field (the string
currently set to that value in the workflow step) to a more descriptive title
that clearly identifies what is executed, for example mentioning
"numbast_extensions", "thirdparty", and "CCCL" so logs show something like "Run
numbast_extensions third-party CCCL tests"; keep the command (python -m pytest
numbast_extensions/tests/thirdparty/CCCL/) unchanged.
In `@ast_canopy/ast_canopy/decl.py`:
- Line 303: Fix the typo in the docstring that reads "Struct/class method who's
name may include template parameters." — change "who's" to the possessive
"whose" in the triple-quoted docstring in decl.py (the docstring that begins
with "Struct/class method who's name may include template parameters.") so it
reads "Struct/class method whose name may include template parameters."
In `@numbast_extensions/tests/thirdparty/CCCL/test_block_scan.py`:
- Around line 16-28: Extract the duplicated helper make_bindings into a shared
test utility (e.g., add it to conftest.py or test_utils.py) and update both
test_block_scan.py and test_block_load_store.py to import and call that shared
make_bindings instead of defining it locally; ensure the extracted function
preserves the same signature and behavior (uses
ast_canopy.parse_declarations_from_source and bind_cxx_class_templates, returns
the matching class by ct.__name__) so tests continue to work unchanged.
In `@numbast/src/numbast/class_template.py`:
- Around line 127-128: Remove the debug print statements in class_template.py
(e.g., the print(f"CTOR SHIM: {shim}") and the other prints around lines 605,
683, 811-813); replace them with proper logging calls such as logging.debug(...)
or simply delete them if not needed, ensuring you import and use the
module-level logger (via import logging and logging.getLogger(__name__)) and
reference the same contextual variables (like shim) in the debug message so
runtime stdout is not polluted.
- Line 59: The declared type hint for ConcreteTypeCache is incorrect: the cache
holds nbtypes.TypeRef objects returned by struct_type_from_instantiation, not
nbtypes.Type. Update the annotation for ConcreteTypeCache to use nbtypes.TypeRef
(or a suitable union including TypeRef) instead of nbtypes.Type so the hint
matches the actual stored values (refer to ConcreteTypeCache and
struct_type_from_instantiation to locate the code).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ast_canopy/ast_canopy/decl.py (1)
545-552: Validate template-argument count to avoid silent truncation.
zipwill drop extra actual template arguments without warning, which can produce an incorrect specialization. Add a length check before buildingkwargs.✅ Suggested fix
targ_names: list[str] = [ tp.name for tp in class_template.template_parameters ] targ_values: list[str] = actual_template_arguments + if len(targ_values) != len(targ_names): + raise ValueError( + f"Expected {len(targ_names)} template arguments, got {len(targ_values)}" + ) kwargs = dict(zip(targ_names, targ_values)) self._instantiation.instantiate(**kwargs)
🤖 Fix all issues with AI agents
In `@numbast/src/numbast/class_template.py`:
- Around line 917-928: generic_resolve currently checks method_templates before
templated_method_to_template causing templated overloads to be silently
shadowed; update generic_resolve to detect a name collision between
method_templates and templated_method_to_template (e.g. attr present in both)
before returning a resolved type and raise a clear error (ValueError or custom)
that includes the attribute name and both sources; reference the existing
helpers _templated_method_ty and _method_ty so the detection runs prior to
calling them and ensure tests cover the collision path.
- Around line 689-789: The regex _CXX_ARRAY_TYPE_RE only matches numeric extents
so array-ref types like T(&)[N] or T(&)[Enum::k] aren't recognized; update the
pattern (used by _make_templated_method_shim_arg_strings) to accept any non-]
content inside brackets (e.g. change (\[\d+\])+ to (\[[^\]]+\])+) so
identifiers, scoped names and expressions are captured as sizes, keep the same
"base" and "sizes" group semantics, and ensure downstream logic (base/sizes
extraction and comparisons around base_norm/nb_base) still works with the
broader sizes token (no further parsing of sizes required for this shim logic).
…s in concrete method and templated method, (2) handle fixed sized array reference inputs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@numbast/src/numbast/class_template.py`:
- Around line 675-686: The error currently conflates "no match" and "ambiguous"
when len(candidates) != 1; update the branching around candidates, overrides,
and intent_errors to produce distinct messages: first keep the existing
overrides-specific TypeError when overrides is not None and not candidates and
intent_errors, then if not candidates raise a TypeError like "No matching
overload found for {qualname} with {arity} args. Overload arities:
{[len(m.function.params) for m in overloads]}", else if len(candidates) > 1
raise a TypeError like "Ambiguous overload for {qualname} with {arity} args.
Matching overload arities: {[len(m.function.params) for m in candidates]}" and
finally return candidates[0].
- Around line 532-626: The code registers a lowering in generic() with
`@lower`(qualname, recvr, *param_types) each time generic() runs, causing
duplicate registrations (and duplicate mangled names via deduplicate_overloads)
for identical signatures; fix by adding a cache keyed by (qualname, recvr,
tuple(param_types)) that records when a lowering has already been registered and
skip re-registering if present. Locate the lowering site in the generic()
function where `@lower`(...) is applied and use a module-level or class-level dict
(or functools.lru_cache-style set) to track registrations before calling
deduplicate_overloads/shim creation and the `@lower` wrapper, so subsequent calls
with the same key return the already-registered FunctionCallConv or no-op
instead of re-decorating.
… given no matching overloads or ambiguous overload
…hod availability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@numbast/src/numbast/class_template.py`:
- Around line 566-596: The pass_ptr_mask misalignment comes from using two
iterators (visible_iter and visible_mask_iter) that can get out of sync when
out-params are lifted; replace the iterator approach with an explicit index over
the visible parameters: keep a visible_idx integer and, when iterating orig_idx
over method_plan.intents, if orig_idx is an out-return use
out_return_types[out_pos] and append False to pass_ptr_mask_for_shim_list,
otherwise read param_types_inner[visible_idx] and
method_plan.pass_ptr_mask[visible_idx], append them and then increment
visible_idx; update the block using param_types_inner, visible_iter, and
visible_mask_iter to use visible_idx and method_plan.pass_ptr_mask indexing
instead to ensure masks align with visible params.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@numbast/tests/test_class_template.py`:
- Around line 172-176: The class docstring in test_class_template.py mentions
"Those tests are xfail to document the gap without implementing it" but no
`@pytest.mark.xfail` tests exist; either add xfail-marked test stubs (e.g., create
tests named test_templated_method_missing_api_xfail and
test_templated_method_paramization_xfail decorated with pytest.mark.xfail and a
short reason) to document the missing user-facing API for templated-method
parameters, or update the docstring to remove or reword that sentence to
accurately state that only default-parameter scenarios (like the "defaults
exist" and arg_intent tests) are covered.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@numbast/tests/test_class_template.py`:
- Around line 39-51: The _sample_class_template_templated_methods fixture
centralizes setup that is only used by a few tests; remove this fixture and
inline its setup directly into each test that currently depends on it by copying
the logic that calls parse_declarations_from_source(...), constructs
MemoryShimWriter(...), and invokes bind_cxx_class_templates(...); update those
tests to return or assign the resulting apis and shim_writer locally instead of
relying on the fixture, and delete the now-unused
_sample_class_template_templated_methods fixture to keep tests independent and
easier to evolve.
Comment on lines
+39
to
+51
| @pytest.fixture | ||
| def _sample_class_template_templated_methods(): | ||
| """Bindings fixture for class-template + templated-method unit tests.""" | ||
| DATA_FOLDER = os.path.join(os.path.dirname(__file__), "data") | ||
| p = os.path.join(DATA_FOLDER, "sample_class_template_templated_method.cuh") | ||
| decls = parse_declarations_from_source(p, [p], "sm_80", verbose=True) | ||
| shim_writer = MemoryShimWriter(f'#include "{p}"') | ||
|
|
||
| apis = bind_cxx_class_templates( | ||
| decls.class_templates, header_path=p, shim_writer=shim_writer | ||
| ) | ||
|
|
||
| return apis, shim_writer |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer inlining setup in tests over a shared fixture here.
This fixture is only used by a small set of tests and can make independent evolution harder. Consider inlining the setup in each test unless there’s a strong DRY benefit.
Based on learnings: In Python tests under numbast/tests, prefer duplicating setup within individual test cases instead of extracting shared fixtures when the goal is to allow independent evolution of tests.
🤖 Prompt for AI Agents
In `@numbast/tests/test_class_template.py` around lines 39 - 51, The
_sample_class_template_templated_methods fixture centralizes setup that is only
used by a few tests; remove this fixture and inline its setup directly into each
test that currently depends on it by copying the logic that calls
parse_declarations_from_source(...), constructs MemoryShimWriter(...), and
invokes bind_cxx_class_templates(...); update those tests to return or assign
the resulting apis and shim_writer locally instead of relying on the fixture,
and delete the now-unused _sample_class_template_templated_methods fixture to
keep tests independent and easier to evolve.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a demo PR that shows a "Parent-Child" pattern that shows an end-to-end use case of class template instantiation, template argument deduction and overload selection working hand in hand.
arg_intentsupport.NullType/opaque fallback, quieter parse defaults).Summary by CodeRabbit
New Features
Improvements
Tests
Chores
Documentation