Support @property getters for warp.struct#1155
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCollects Python Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Kernel as "Kernel (source)"
participant Codegen as "Codegen / emit_Attribute"
participant StructType as "Struct (type) / Struct.properties"
participant PropFunc as "Generated Property Function"
participant Runtime as "Runtime / Native Function"
Kernel->>Codegen: emit attribute read for `s.neg_value`
Note right of Codegen: detect aggregate is Struct\nlookup `neg_value` in StructType.properties
Codegen->>PropFunc: emit add_call, bind self to struct instance
PropFunc->>Runtime: invoke native property getter
Runtime-->>PropFunc: return value
PropFunc-->>Codegen: provide value expression
Codegen-->>Kernel: inline call result into kernel code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
| for name, item in property_members: | ||
| # We currently support only getters | ||
| if item.fset is not None: | ||
| raise TypeError("Struct properties with setters are not supported") |
There was a problem hiding this comment.
For now, only getters are supported. Setters should be doable too.
There was a problem hiding this comment.
Without setters, this feature feels incomplete.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
warp/tests/test_struct.py (1)
912-921: Add backward pass test for property differentiation.The test validates forward pass execution but doesn't verify that properties work correctly in the backward pass. Since properties are converted to Warp functions, they should be differentiable. Consider adding a gradient test to ensure
d(neg_value)/d(value) = -1:Suggested additional test
def test_struct_property_grad(test, device): s = StructWithProperty() s.value = wp.array([42.0], dtype=float, device=device, requires_grad=True) out = wp.zeros(1, dtype=float, device=device, requires_grad=True) tape = wp.Tape() with tape: @wp.kernel def grad_test_kernel(s: StructWithProperty, out: wp.array(dtype=float)): out[0] = s.neg_value wp.launch(grad_test_kernel, dim=1, inputs=[s, out], device=device) tape.backward(loss=out) # d(neg_value)/d(value) should be -1 assert_np_equal(s.value.grad.numpy(), np.array([-1.0]))Note: This assumes the property can access array fields. If that's not supported yet, use a simpler struct with scalar fields, or defer this test if array field access in properties is out of scope.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
warp/_src/codegen.pywarp/tests/test_struct.py
🧰 Additional context used
🧬 Code graph analysis (2)
warp/_src/codegen.py (1)
warp/_src/context.py (3)
context(3470-3485)Function(132-528)func(862-905)
warp/tests/test_struct.py (1)
warp/_src/context.py (4)
struct(1320-1342)kernel(1174-1316)launch(6594-6635)launch(6638-6877)
🪛 Ruff (0.14.10)
warp/_src/codegen.py
526-526: Avoid specifying long messages outside the exception class
(TRY003)
528-528: Avoid specifying long messages outside the exception class
(TRY003)
warp/tests/test_struct.py
912-912: Unused function argument: test
(ARG001)
920-920: assert_np_equal may be undefined, or defined from star imports
(F405)
923-923: add_function_test may be undefined, or defined from star imports
(F405)
🔇 Additional comments (8)
warp/tests/test_struct.py (3)
898-905: LGTM! Clean property definition.The struct definition is straightforward and serves as a good test case for the property feature. The property correctly uses the
@propertydecorator and returns a derived value from the struct field.
907-910: LGTM! Property access test.The kernel correctly demonstrates property access via dot notation (
s.neg_value), which is the key feature being tested.
923-923: LGTM! Standard test registration.The test registration follows the established pattern in this file. The static analysis warning about
add_function_testbeing undefined is a false positive due to the star import fromwarp.tests.unittest_utilson line 24.warp/_src/codegen.py (5)
463-463: LGTM! Properties dictionary added.The initialization of
self.propertiesfollows the same pattern asself.varsand includes a helpful type hint.
486-490: LGTM! Property metadata collection.The early collection of property members with deferred Function creation is a sound design. The comment clearly explains why Function creation is postponed until after
native_nameis computed.
512-515: Property names added to hash.Including property names in the struct hash ensures module rebuilds when property names change. The property implementations will be hashed separately via the Function class mechanism, so implementation changes should also trigger rebuilds.
521-555: Property Function creation logic is sound.The implementation correctly:
- Validates that only getters are supported (no setters/deleters)
- Adds type annotations for the
selfargument to enable overload resolution- Creates unique Function objects with predictable C++ names (
{struct_native_name}_{property_name})- Handles edge cases like missing
__annotations__attributeThe static analysis warnings about long error messages (lines 526, 528) are minor style suggestions that don't affect functionality.
2309-2313: LGTM! Property access code generation.The property access is cleanly integrated into the existing attribute resolution logic. Calling the property Function via
add_callensures that both forward and adjoint code are generated correctly, enabling automatic differentiation through properties.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Added support for @property getters in warp.struct classes, allowing computed properties to be accessed within Warp kernels by converting property getters into Warp functions at codegen time.
Key changes:
- Added
propertiesdict toStructclass to store property-to-Function mappings - Property collection using
inspect.getmembers()during struct initialization - Property names included in struct hash for identity tracking
- Property getters converted to Warp Functions with struct instance as first argument
- Property access handled in
Adjoint.emit_Attribute()before regular field access - Added validation to reject properties with setters or deleters
- Test coverage for basic property getter functionality
Minor issues:
- Misleading comment about hash stability at line 512
- Missing validation for edge case where property getter has no arguments
Confidence Score: 4/5
- This PR is safe to merge with minor documentation improvements recommended
- The implementation is solid and well-structured, with proper validation for setters/deleters and correct integration into the attribute resolution pipeline. Test coverage demonstrates basic functionality. Minor issues include a misleading comment and a missing edge case validation, neither of which would cause runtime errors in normal usage.
- No files require special attention - the implementation is straightforward and well-integrated
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| warp/_src/codegen.py | 4/5 | Adds @property support for warp.struct by collecting properties during struct initialization, converting them to Warp Functions, and handling property access in attribute resolution |
| warp/tests/test_struct.py | 5/5 | Adds basic test for property getter functionality with a simple struct that has a neg_value property returning the negated value |
Sequence Diagram
sequenceDiagram
participant User
participant Decorator as @wp.struct
participant Struct as Struct.__init__
participant InspectModule as inspect.getmembers
participant FunctionClass as Function
participant Adjoint as Adjoint.emit_Attribute
User->>Decorator: Define struct with @property
Decorator->>Struct: Create Struct instance
Struct->>Struct: Collect field annotations
Struct->>InspectModule: getmembers(cls)
InspectModule-->>Struct: Return all members including properties
Struct->>Struct: Filter for property instances
Struct->>Struct: Compute hash (includes property names)
Struct->>Struct: Set native_name
loop For each property
Struct->>Struct: Validate no setter/deleter
Struct->>Struct: Add self annotation to getter
Struct->>FunctionClass: Create Function from getter
FunctionClass-->>Struct: Return Function instance
Struct->>Struct: Set native_func name
Struct->>Struct: Store in properties dict
end
Note over User,Adjoint: Later: During kernel compilation
User->>Adjoint: Access struct.property in kernel
Adjoint->>Adjoint: Check if attr is property
alt Is property
Adjoint->>Adjoint: add_call(prop, (aggregate,), {}, {})
Adjoint-->>User: Return property value
else Is regular field
Adjoint->>Adjoint: Access field normally
Adjoint-->>User: Return field value
end
Additional Comments (2)
|
antoinebrl
left a comment
antoinebrl
left a comment
There was a problem hiding this comment.
Would it make sense to augment the documentation to illustrate this new feature?
Greptile SummaryThis PR adds support for Key changes:
The latest commit reverted from copying annotations to directly mutating Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Decorator as @wp.struct Decorator
participant Struct as Struct.__init__
participant PropValidation as Property Validation
participant FuncCreation as Function Creation
participant Kernel as Kernel Execution
participant Adjoint as Adjoint.eval_attribute
User->>Decorator: Define struct with @property
Decorator->>Struct: Initialize Struct(key, cls, module)
Struct->>Struct: Collect vars from annotations
Struct->>PropValidation: Scan for property members
PropValidation->>PropValidation: Check for name conflicts with fields
PropValidation->>PropValidation: Check for setters/deleters (reject if found)
PropValidation->>Struct: Store property_members list
Struct->>Struct: Compute hash (include property names)
Struct->>Struct: Generate native_name
Struct->>FuncCreation: Create Function for each property
FuncCreation->>FuncCreation: Add self annotation to getter
FuncCreation->>FuncCreation: Set unique native_func name
FuncCreation->>Struct: Store in self.properties dict
Struct->>Decorator: Return configured Struct
Decorator->>User: Return decorated class
User->>Kernel: Launch kernel with struct.property access
Kernel->>Adjoint: Evaluate attribute access (s.neg_value)
Adjoint->>Adjoint: Check if attr in aggregate_type.properties
Adjoint->>Adjoint: Call property Function with struct as argument
Adjoint->>Kernel: Return computed value
Kernel->>User: Return result
|
There was a problem hiding this comment.
Additional Comments (3)
-
warp/_src/codegen.py, line 532-539 (link)logic: Mutating
getter.__annotations__directly modifies the original function's annotations, which could cause issues if the same property getter is used in multiple structs or accessed outside the struct context. Should create a copy of annotations before modifying. -
warp/_src/codegen.py, line 536-539 (link)logic: Property getters without a
selfparameter will silently fail to get proper type annotations, leading to confusing errors during Function creation. Should validate that property getters have at least one parameter. -
warp/tests/test_struct.py, line 899-924 (link)style: Test coverage is minimal - only validates basic property getter. Missing tests for:
- Multiple properties in same struct
- Properties with complex types (structs, arrays)
- Error cases (setter/deleter rejection)
- Property calling other properties
- Properties with no return type annotation
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 files reviewed, 3 comments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
warp/_src/codegen.py (1)
521-555: Property Function creation is well-structured.The implementation correctly:
- Validates that only getters are supported (no setters/deleters)
- Injects the struct type as the
selfparameter annotation- Creates a unique native function name based on struct and property names
One consideration: modifying
getter.__annotations__directly at line 539 mutates the original function's annotations. If the same getter function is reused elsewhere, this could cause unexpected behavior. Consider working with a copy:♻️ Optional: Avoid mutating original function annotations
# Find the name of the first argument (conventionally 'self') argspec = inspect.getfullargspec(getter) if len(argspec.args) > 0: self_arg = argspec.args[0] - getter.__annotations__[self_arg] = self + # Use a copy to avoid mutating the original function's annotations + if not hasattr(getter, "__annotations__"): + getter.__annotations__ = {} + else: + getter.__annotations__ = getter.__annotations__.copy() + getter.__annotations__[self_arg] = self
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
warp/_src/codegen.pywarp/tests/test_struct.py
🧰 Additional context used
🧬 Code graph analysis (1)
warp/tests/test_struct.py (1)
warp/_src/context.py (4)
struct(1320-1342)kernel(1174-1316)launch(6594-6635)launch(6638-6877)
🪛 Ruff (0.14.10)
warp/_src/codegen.py
526-526: Avoid specifying long messages outside the exception class
(TRY003)
528-528: Avoid specifying long messages outside the exception class
(TRY003)
warp/tests/test_struct.py
913-913: Unused function argument: test
(ARG001)
921-921: assert_np_equal may be undefined, or defined from star imports
(F405)
924-924: Unused function argument: device
(ARG001)
940-940: add_function_test may be undefined, or defined from star imports
(F405)
941-941: add_function_test may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (8)
warp/tests/test_struct.py (4)
899-910: Well-structured test struct and kernel for property access.The struct definition with the
@propertydecorator and the corresponding kernel that readss.neg_valuecorrectly exercise the new property getter functionality.
913-921: Test correctly validates property getter in kernel.The test instantiates the struct, sets a value, launches the kernel that reads the property, and validates the output. This provides good coverage for the basic property getter flow.
924-937: Good addition of setter rejection test.This test validates that attempting to define a property with a setter raises an appropriate
TypeErrorwith a clear message. This addresses the previous review feedback about testing setter rejection.
940-941: Test registrations look correct.Both new tests are properly registered with
add_function_testand include thedevicesparameter for cross-device testing.warp/_src/codegen.py (4)
463-463: Correct initialization of properties dictionary.The
propertiesdict is properly initialized as empty and will be populated afternative_nameis computed, following the deferred initialization pattern described in the AI summary.
486-490: Property collection correctly defers Function creation.Collecting property metadata early (before hash computation) while deferring Function creation until after
native_nameis set is the right approach. This ensures property names are included in the hash while avoiding issues with forward references.
512-515: Property names correctly included in struct hash.Hashing property names ensures the struct identity changes if properties are renamed. Note that changes to property getter implementation won't affect the hash, which is consistent with how struct field changes are handled.
2309-2312: Property dispatch in emit_Attribute correctly handles property access.The implementation properly checks if the accessed attribute is a property on the struct and dispatches to the corresponding Function via
add_call. This integrates cleanly with the existing attribute access logic.
There was a problem hiding this comment.
Additional Comments (1)
-
warp/tests/test_struct.py, line 925 (link)syntax: error message regex doesn't match actual error in
codegen.py:526
2 files reviewed, 1 comment
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds @property getter support for warp.struct, converting properties into warp functions at codegen. Properties are collected via inspect.getmembers(), validated (rejecting setters/deleters), and stored as Function objects. During attribute access, properties are dispatched through add_call() instead of direct field access.
Key Changes:
- Added
Struct.propertiesdict to track property Functions - Property names hashed into struct identity (
native_name) - Property access handled in
emit_Attribute()via function call dispatch - Validation rejects setters and deleters at struct definition time
Issues Found:
- Test error message regex doesn't match implementation ("do not support" vs "with setters are not supported")
- Uses
inspect.getfullargspec()instead ofget_full_arg_spec()helper, risking stringized annotation issues - Only property names hashed, not return types (inconsistent with field hashing)
- No test coverage for property assignment attempts inside kernels
Confidence Score: 3/5
- This PR has implementation issues that need fixing before merge
- Score reflects one critical test failure (regex mismatch will cause test to fail), plus several code quality and consistency issues. The core feature implementation is sound, but the test bug must be fixed.
- Pay close attention to the test regex fix in
warp/tests/test_struct.pyand consider the annotation handling improvement inwarp/_src/codegen.py
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| warp/_src/codegen.py | 3/5 | Added property support for warp structs; found issues with error message mismatch, potential annotation handling bug, and missing assignment validation |
| warp/tests/test_struct.py | 3/5 | Added basic property tests; error message regex doesn't match actual error message in implementation |
Sequence Diagram
sequenceDiagram
participant User
participant Decorator as @wp.struct
participant Struct as Struct.__init__
participant inspect
participant Function
participant Kernel as @wp.kernel
participant Adjoint as emit_Attribute
User->>Decorator: Define struct with @property
Decorator->>Struct: Create Struct instance
Struct->>inspect: getmembers(cls)
inspect-->>Struct: property_members list
Struct->>Struct: Hash fields and property names
Struct->>Struct: Set native_name
loop For each property
Struct->>inspect: getfullargspec(getter)
Struct->>Struct: Add self annotation
Struct->>Function: Create Function(getter)
Function-->>Struct: Property function
Struct->>Struct: Store in self.properties
end
User->>Kernel: Access struct.property
Kernel->>Adjoint: emit_Attribute(node)
Adjoint->>Adjoint: Check if attr in properties
Adjoint->>Adjoint: add_call(prop_func, args)
Adjoint-->>Kernel: Return property value
Additional Comments (4)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
warp/tests/test_struct.py (2)
925-939: LGTM! Test correctly validates setter rejection.The test properly verifies that defining a property with a setter raises a
TypeErrorwith an appropriate message. This addresses the past review feedback requesting this validation.Optional: Consider removing unused
deviceparameter.The
deviceparameter on line 925 is not used in this test since it only validates struct definition, not kernel execution. While keeping it maintains signature consistency with other test functions, removing it would eliminate the static analysis warning.♻️ Optional refactor to remove unused parameter
-def test_struct_property_with_setter(test, device): +def test_struct_property_with_setter(test, _device): """Tests that structs with properties (setters) are not supported."""or update the test registration to pass
devices=None:-add_function_test(TestStruct, "test_struct_property_with_setter", test_struct_property_with_setter, devices=devices) +add_function_test(TestStruct, "test_struct_property_with_setter", test_struct_property_with_setter, devices=None)
899-943: Optional: Consider expanding test coverage for edge cases.While the current tests adequately cover the basic property functionality, you might consider adding tests for:
- Multiple properties on a single struct
- Properties that reference multiple fields
- Properties on nested structs
- Property with a deleter (to verify it's handled correctly or rejected)
These are nice-to-have improvements rather than blockers.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
warp/tests/test_struct.py
🧰 Additional context used
🧬 Code graph analysis (1)
warp/tests/test_struct.py (1)
warp/_src/context.py (1)
struct(1320-1342)
🪛 Ruff (0.14.10)
warp/tests/test_struct.py
913-913: Unused function argument: test
(ARG001)
922-922: assert_np_equal may be undefined, or defined from star imports
(F405)
925-925: Unused function argument: device
(ARG001)
942-942: add_function_test may be undefined, or defined from star imports
(F405)
943-943: add_function_test may be undefined, or defined from star imports
(F405)
🔇 Additional comments (4)
warp/tests/test_struct.py (4)
899-906: LGTM! Clean test struct with read-only property.The
StructWithPropertydefinition is well-structured with a simple property getter that returns the negation of thevaluefield. This provides a clear test case for the property feature.
908-910: LGTM! Kernel correctly demonstrates property access.The kernel properly accesses the struct property using dot-notation (
s.neg_value), which aligns with the PR objectives for enabling property access inside kernels.
913-922: LGTM! Test correctly verifies property getter functionality.The test properly validates that property getters work end-to-end: it sets a value, launches a kernel that reads the property, and asserts the correct negated result.
942-943: LGTM! Test registrations are correct.Both tests are properly registered following the standard pattern used throughout the file.
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
Ghelfi
force-pushed
the
support_property
branch
from
997063a to
6168508
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
warp/_src/codegen.py (1)
532-539: Consider copying annotations before mutation.The code modifies
getter.__annotations__directly, which mutates the original function's annotations. While property getters on Warp structs are unlikely to be reused elsewhere, defensively copying the annotations dict before modification would be safer.♻️ Suggested fix
# We need to add 'self' as the first argument, with the type of the struct itself. # This allows overload resolution to match the struct instance to the 'self' argument. - if not hasattr(getter, "__annotations__"): - getter.__annotations__ = {} + # Copy annotations to avoid mutating the original function + annotations = dict(getattr(getter, "__annotations__", {})) # Find the name of the first argument (conventionally 'self') argspec = get_full_arg_spec(getter) if len(argspec.args) > 0: self_arg = argspec.args[0] - getter.__annotations__[self_arg] = self + annotations[self_arg] = self + getter.__annotations__ = annotations
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
warp/_src/codegen.pywarp/tests/test_struct.py
🧰 Additional context used
🧬 Code graph analysis (1)
warp/tests/test_struct.py (2)
warp/_src/context.py (4)
struct(1320-1342)kernel(1174-1316)launch(6594-6635)launch(6638-6877)warp/tests/unittest_utils.py (1)
add_function_test(282-301)
🪛 Ruff (0.14.10)
warp/tests/test_struct.py
913-913: Unused function argument: test
(ARG001)
922-922: assert_np_equal may be undefined, or defined from star imports
(F405)
925-925: Unused function argument: device
(ARG001)
942-942: add_function_test may be undefined, or defined from star imports
(F405)
943-943: add_function_test may be undefined, or defined from star imports
(F405)
warp/_src/codegen.py
526-526: Avoid specifying long messages outside the exception class
(TRY003)
528-528: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (8)
warp/tests/test_struct.py (3)
899-910: LGTM!The struct definition with a read-only property and the corresponding kernel that accesses
s.neg_valueare correctly implemented for testing the new property feature.
913-922: LGTM!The test correctly validates that struct properties (getters) work as expected by verifying the computed negative value.
925-943: LGTM!The test correctly validates that struct properties with setters raise a
TypeError, addressing the previous review feedback. The unuseddeviceparameter is consistent with similar test patterns in this file (e.g.,test_struct_inheritance_error).warp/_src/codegen.py (5)
463-463: LGTM!Proper initialization of the properties dictionary to store generated Warp Functions for struct property getters.
486-490: LGTM!Using
inspect.getmembersto collect properties is appropriate. Deferring Function creation until afternative_nameis computed is the correct approach since the Function's native name depends on it.
512-515: LGTM!Including property names in the hash ensures that adding, removing, or renaming properties invalidates the module hash, triggering recompilation as expected.
545-555: LGTM!The Function creation correctly:
- Uses the property getter as the source function for type inference.
- Sets a unique key
{struct.key}.{property_name}.- Uses an empty namespace to generate a free function.
- Assigns a predictable native function name
{native_name}_{property_name}.
2309-2312: LGTM!The attribute dispatch logic correctly intercepts property access on struct types and routes to the generated property Function via
add_call, seamlessly integrating with existing codegen infrastructure.
warp/_src/codegen.py
Outdated
| if len(argspec.args) > 0: | ||
| self_arg = argspec.args[0] | ||
| getter.__annotations__[self_arg] = self |
There was a problem hiding this comment.
logic: If property getter has no arguments, self annotation won't be added but Function will still be created, potentially causing issues during code generation
| if len(argspec.args) > 0: | |
| self_arg = argspec.args[0] | |
| getter.__annotations__[self_arg] = self | |
| if len(argspec.args) == 0: | |
| raise TypeError(f"Property getter '{name}' must have at least one argument (self)") | |
| self_arg = argspec.args[0] | |
| getter.__annotations__[self_arg] = self |
| for name, item in inspect.getmembers(self.cls): | ||
| if isinstance(item, property): | ||
| property_members.append((name, item)) |
There was a problem hiding this comment.
logic: No validation prevents properties from having the same name as struct fields, which could cause ambiguity since properties are checked before vars in attribute access (line 2309)
| for name, item in inspect.getmembers(self.cls): | |
| if isinstance(item, property): | |
| property_members.append((name, item)) | |
| for name, item in inspect.getmembers(self.cls): | |
| if isinstance(item, property): | |
| if name in self.vars: | |
| raise TypeError(f"Property '{name}' conflicts with struct field of the same name") | |
| property_members.append((name, item)) |
Signed-off-by: Alexandre Ghelfi <alexandre.ghelfi@helsing.ai>
Ghelfi
force-pushed
the
support_property
branch
from
6168508 to
6c226e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
warp/tests/test_struct.py (1)
913-923: Minor: Unusedtestparameter.The test logic is correct and properly verifies that property getters work in kernels. However, the
testparameter is unused in this function (though it may be required by theadd_function_testframework signature).If the framework allows it, consider removing the parameter:
-def test_struct_property(test, device): +def test_struct_property(device):Otherwise, you can prefix it with an underscore to indicate it's intentionally unused:
_test.warp/_src/codegen.py (1)
521-556: Property Function creation is correct with minor style suggestion.The implementation properly:
- Defers Function creation until after
native_nameis available (needed for unique native_func naming)- Correctly rejects setters and deleters with appropriate error messages
- Annotates the getter's first argument with the Struct type for proper overload resolution
- Creates Functions with unique
native_funcnames based on the struct's native_nameThe error messages on lines 526 and 528 could be extracted to constants to follow best practices, but this is a minor style issue.
♻️ Optional: Extract error messages to constants
To follow TRY003 guidelines, consider extracting the error messages:
+# At module level +_STRUCT_PROPERTY_SETTER_ERROR = "Struct properties with setters are not supported" +_STRUCT_PROPERTY_DELETER_ERROR = "Struct properties with deleters are not supported" + # In Struct.__init__: if item.fset is not None: - raise TypeError("Struct properties with setters are not supported") + raise TypeError(_STRUCT_PROPERTY_SETTER_ERROR) if item.fdel is not None: - raise TypeError("Struct properties with deleters are not supported") + raise TypeError(_STRUCT_PROPERTY_DELETER_ERROR)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
warp/_src/codegen.pywarp/tests/test_struct.py
🧰 Additional context used
🧬 Code graph analysis (2)
warp/tests/test_struct.py (1)
warp/_src/context.py (4)
struct(1320-1342)kernel(1174-1316)launch(6594-6635)launch(6638-6877)
warp/_src/codegen.py (1)
warp/_src/context.py (3)
context(3470-3485)Function(132-528)func(862-905)
🪛 Ruff (0.14.10)
warp/tests/test_struct.py
913-913: Unused function argument: test
(ARG001)
922-922: assert_np_equal may be undefined, or defined from star imports
(F405)
925-925: Unused function argument: device
(ARG001)
942-942: add_function_test may be undefined, or defined from star imports
(F405)
943-943: add_function_test may be undefined, or defined from star imports
(F405)
warp/_src/codegen.py
526-526: Avoid specifying long messages outside the exception class
(TRY003)
528-528: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
warp/tests/test_struct.py (3)
899-906: LGTM! Clean test struct definition.The
StructWithPropertyprovides a clear test case for read-only property getters on structs.
908-911: LGTM! Correct kernel implementation.The kernel properly accesses the struct property and demonstrates the expected usage pattern.
925-943: Test logic is correct.The test properly verifies that struct properties with setters are rejected with the appropriate error message. The unused
deviceparameter (line 925) is likely required by theadd_function_testframework signature, since the test is registered withdevices=devices(line 943).warp/_src/codegen.py (3)
463-463: LGTM! Property storage correctly initialized.The
propertiesdictionary is properly initialized to store the mapping from property names to their corresponding Warp Functions.
486-516: LGTM! Property collection and hashing correctly implemented.The implementation properly:
- Collects property metadata early using
inspect.getmembers- Includes property names in the struct hash to ensure identity stability
- Computes the hash before
native_name, which depends on itThis ensures that adding, removing, or renaming properties will result in a different struct hash, which is the correct behavior.
2309-2313: LGTM! Property attribute dispatch correctly implemented.The code properly routes struct property reads to the corresponding Warp Functions:
- Checks that the aggregate is a Struct and the attribute is in its properties
- Uses
add_callto invoke the property Function with the struct instance as the first argument- This integrates seamlessly with the existing attribute resolution flow
warp/_src/codegen.py
Outdated
| if not hasattr(getter, "__annotations__"): | ||
| getter.__annotations__ = {} | ||
|
|
||
| # Find the name of the first argument (conventionally 'self') | ||
| argspec = get_full_arg_spec(getter) | ||
| if len(argspec.args) > 0: | ||
| self_arg = argspec.args[0] | ||
| getter.__annotations__[self_arg] = self |
There was a problem hiding this comment.
Directly mutating the original getter function's __annotations__ dictionary is problematic. If the struct class is instantiated multiple times or the getter function is used elsewhere, this mutation will affect all instances and could cause unexpected behavior or conflicts.
Consider copying the annotations dictionary instead:
| if not hasattr(getter, "__annotations__"): | |
| getter.__annotations__ = {} | |
| # Find the name of the first argument (conventionally 'self') | |
| argspec = get_full_arg_spec(getter) | |
| if len(argspec.args) > 0: | |
| self_arg = argspec.args[0] | |
| getter.__annotations__[self_arg] = self | |
| # We need to add 'self' as the first argument, with the type of the struct itself. | |
| # This allows overload resolution to match the struct instance to the 'self' argument. | |
| # Copy annotations to avoid mutating the original function | |
| annotations = dict(getattr(getter, "__annotations__", {})) | |
| # Find the name of the first argument (conventionally 'self') | |
| argspec = get_full_arg_spec(getter) | |
| if len(argspec.args) > 0: | |
| self_arg = argspec.args[0] | |
| annotations[self_arg] = self | |
| getter.__annotations__ = annotations |
Signed-off-by: Alexandre Ghelfi <alexandre.ghelfi@helsing.ai>
warp/_src/codegen.py
Outdated
| annotations = dict(getattr(getter, "__annotations__", {})) | ||
|
|
||
|
|
||
| # Find the name of the first argument (conventionally 'self') | ||
| argspec = get_full_arg_spec(getter) | ||
|
|
||
| if len(argspec.args) == 0: | ||
| raise TypeError(f"Struct property '{name}' must have at least one argument (self)") | ||
| self_arg = argspec.args[0] | ||
| annotations[self_arg] = self | ||
| getter.__annotations__ = annotations |
There was a problem hiding this comment.
logic: Copying annotations on line 535 but still mutating the original getter.__annotations__ on line 545 defeats the purpose. Should assign the copied dict back instead:
| annotations = dict(getattr(getter, "__annotations__", {})) | |
| # Find the name of the first argument (conventionally 'self') | |
| argspec = get_full_arg_spec(getter) | |
| if len(argspec.args) == 0: | |
| raise TypeError(f"Struct property '{name}' must have at least one argument (self)") | |
| self_arg = argspec.args[0] | |
| annotations[self_arg] = self | |
| getter.__annotations__ = annotations | |
| # Copy annotations to avoid mutating the original function | |
| annotations = dict(getattr(getter, "__annotations__", {})) | |
| # Find the name of the first argument (conventionally 'self') | |
| argspec = get_full_arg_spec(getter) | |
| if len(argspec.args) == 0: | |
| raise TypeError(f"Struct property '{name}' must have at least one argument (self)") | |
| self_arg = argspec.args[0] | |
| annotations[self_arg] = self |
Signed-off-by: Alexandre Ghelfi <alexandre.ghelfi@helsing.ai>
nvlukasz
left a comment
nvlukasz
left a comment
There was a problem hiding this comment.
Thanks @Ghelfi, it looks like a potentially useful feature, but there are some downsides.
I agree that foo.bar is simpler and easier to read/write than get_bar(foo), but it's just syntactic sugar. Without setters, adding properties to structs feels incomplete. One concern is that we're adding complexity to the hashing and codegen. There are already some subtle problems with the implementation, like hashing not taking into account the function source.
Warp structs were originally intended to be simple POD (plain-old-data) types, without any higher-level OO semantics. I'm concerned that by adding properties we are venturing into OO territory that can end up being burdensome. If we add properties, why not methods? After all, properties are just methods in disguise. And then inheritance etc... Slippery slope :)
Of course, we can draw the line at just properties, perhaps even just getters. I'm not entirely against the idea, but it's something that needs a bit more thought and more extensive testing. For example, we should make sure that everything works fine with auto-differentiation. Should we allow custom gradient and replay function on properties? Things can get gnarly there.
Curious to hear @mmacklin's perspective.
| self.hash = ch.digest() | ||
| # Hash property names (to ensure layout/identity stability if names change) | ||
| for name, _ in property_members: | ||
| ch.update(bytes(name, "utf-8")) |
There was a problem hiding this comment.
This only considers the name, but it should also include the function source. Otherwise modifying the getter won't trigger recompilation.
| # in C++ that takes the struct as the first argument (e.g., StructName_propName(struct_inst)). | ||
| p_func = warp._src.context.Function( | ||
| func=getter, | ||
| key=f"{self.key}.{name}", |
There was a problem hiding this comment.
The key shouldn't include the . character (or other non-identifier characters). Can use underscore instead.
| ) | ||
|
|
||
| # Ensure the C++ function name is unique and predictable | ||
| p_func.native_func = f"{self.native_name}_{name}" |
There was a problem hiding this comment.
This should be passed to the Function constructor as native_func=....
| for name, item in property_members: | ||
| # We currently support only getters | ||
| if item.fset is not None: | ||
| raise TypeError("Struct properties with setters are not supported") |
There was a problem hiding this comment.
Without setters, this feature feels incomplete.
| # Find the name of the first argument (conventionally 'self') | ||
| argspec = get_full_arg_spec(getter) | ||
|
|
||
| if len(argspec.args) == 0: |
There was a problem hiding this comment.
Does it make sense to allow more than one argument for the getter? How should that work? Would those need to be annotated by the user? I think it would currently fail during codegen without additional logic.
Might be better to reject any function that doesn't have exactly one argument here.
3 participants
Description
Add support for
@propertyinwarp.struct.Allows user to define property getters in the warp.struct. The properties are converted into warp functions at the codegen step.
Closes #1152
Before your PR is "Ready for review"
__init__.pyi,docs/api_reference/,docs/language_reference/)pre-commit run -aSummary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.