[Lang] W3 Accept numpy/torch external arrays at qd.Tensor slots - #878
[Lang] W3 Accept numpy/torch external arrays at qd.Tensor slots#878hughperkins wants to merge 9 commits into
Conversation
A qd.Tensor kernel-arg slot previously routed only Ndarray/AnyArray values through the ndarray feature path; numpy arrays and torch tensors fell through to the template path, where torch produced one specialization per tensor instance and numpy raised "unhashable type: 'numpy.ndarray'". Route external arrays through the same ndarray path as a bare qd.ndarray, keyed by dtype and rank. The predicate is a positive numpy.ndarray / torch.Tensor allowlist rather than a duck-typed .shape/.dtype check: those are exactly the external types the launch path (FuncBase._recursive_set_args) can bind, and a duck-typed check would wrongly divert quadrants-native Field/SNode (which also expose .shape/.dtype) off the template path.
Assert that numpy and torch each collapse to a single specialization (not one per instance), that numpy no longer raises "unhashable type", and that a Field through the same qd.Tensor slot still takes the template path.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4d0ba18dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Address codex review: reuse of a compiled specialization for an external array requires matching gradient state (a torch tensor's requires_grad), not only dtype and rank - requires_grad is folded into the specialization key.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33359ecd84
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
hughperkins
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Will ignore doc quality chekcs for tensor.md, since handled separately by #881 (unelss that pr merges first). |
…rays # Conflicts: # python/quadrants/lang/_template_mapper_hotpath.py
|
Running genesis benchamrks and unit tests |
|
Genesis bnechmarks look okish: 
|
|
Agent addressing doc quality |
The doc-quality CI flagged `qd.types.ndarray()` and the term "slot" on the external-array paragraph as undefined jargon for end users. The sentence was a redundant cross-reference; the paragraph already states that external arrays dispatch through the same ndarray path as a bare qd.ndarray, so drop it.
|
Genesis unit tests passing: 
|
hughperkins
added
awaiting review pass
|
|
||
| For a parameter annotated `qd.Tensor`, calls to the same kernel may alternate among a wrapper, its bare field or ndarray implementation, and `None`. If callers can pass `None`, guard tensor operations with [`qd.static()`](static.md), for example `if qd.static(x is not None):`; the guarded operations are not traced for the `None` specialization. The parameter remains annotated `qd.Tensor`, and `None` must be passed explicitly. | ||
|
|
||
| A `qd.Tensor` parameter also accepts **external arrays** - `numpy.ndarray` and `torch.Tensor` - dispatched through the same ndarray path as a bare `qd.ndarray`, i.e. specialized by dtype, rank and gradient state rather than by object identity. Distinct arrays that agree on all three therefore reuse one compiled kernel; differing on any of them (including a torch tensor's `requires_grad`, which is folded into the specialization) produces a separate one. Other Python objects that merely expose `.shape` / `.dtype` (a `qd.field`, for instance) are not treated as external arrays and keep following the template path. |
There was a problem hiding this comment.
not clear waht "dispatched through the same ndarray path as a bare `qd.ndarray" means. Do we need this phrase at all?
What does it mean 'follow the template path'?
There was a problem hiding this comment.
Agent says:
Reworded in 4092837 to drop both internal phrases. It now describes the behavior directly: an external array is specialized by its dtype, rank and gradient state (not by the array object's identity), so arrays sharing those reuse one compiled kernel; and objects that merely expose .shape / .dtype (like a qd.field) are handled like any other qd.Tensor argument. No more "ndarray path" / "template path".
| """Whether ``arg`` is an external array (numpy / torch) that a ``qd.Tensor`` slot should route through the | ||
| ndarray feature path instead of the template path. | ||
|
|
||
| Positive allowlist of exactly ``numpy.ndarray`` and ``torch.Tensor``: these are the only external array types |
There was a problem hiding this comment.
this whole paragraph "Positive allowlist of exactly numpy.ndarray and torch.Tensor: these are the only external array types
the launch path (FuncBase._recursive_set_args) can actually bind, so anything else would specialize here
only to raise there. A looser duck-typed check on .shape / .dtype is unsafe because quadrants-native
Field and SNode both expose those attributes and must keep taking the template path. torch is
resolved via sys.modules so this stays free of a hard torch dependency and costs nothing when torch was
never imported." seems superflous?
There was a problem hiding this comment.
Agent says:
Agreed - removed the whole _is_external_array docstring in 4092837. The one load-bearing point (allowlist numpy/torch only, because Field/SNode also expose .shape / .dtype so a duck-typed check would misroute them) is covered by the design doc and this PR discussion rather than inline. Happy to add a one-line comment back if you'd prefer it kept near the code.
|
|
||
|
|
||
| def _is_external_array(arg: Any) -> bool: | ||
| """Whether ``arg`` is an external array (numpy / torch) that a ``qd.Tensor`` slot should route through the |
There was a problem hiding this comment.
this comment seems superfluous. We know from the function name what it does I feel. Remove "Whether arg is an external array (numpy / torch) that a qd.Tensor slot should route through the ndarray feature path instead of the template path."
There was a problem hiding this comment.
Agent says:
Done - removed in 4092837. The entire docstring is gone (per the sibling comment on the same function).
|
|
||
|
|
||
| # ---------------------------------------------------------------------------- | ||
| # External arrays (numpy / torch) at a qd.Tensor slot (issue #856, W3). |
There was a problem hiding this comment.
why are we documenting this in tests? Seems superfluous. Also, dont write history, like 'before this' etc. Lets just remove "# A qd.Tensor slot routes numpy arrays and torch tensors through the ndarray feature path (keyed by dtype/ndim),
exactly like qd.types.ndarray() already does, instead of falling through to the template path. Before this,
torch produced one specialization per tensor instance and numpy raised ``TypeError: unhashable type:
'numpy.ndarray'. Field and scalar template values must be unaffected: Field also exposes .shape/.dtype``,
so the predicate is a positive numpy/torch allowlist rather than a duck-typed check."
There was a problem hiding this comment.
Agent says:
Removed in 4092837 - dropped the design/history block (including the "before this" wording), leaving just a one-line section header.
|
Need to get agent to address some commetns I've left. |
Address review comments: reword the tensor.md external-array paragraph to drop the internal "ndarray path" / "template path" phrasing, remove the _is_external_array docstring, and drop the design/history comment block in test_tensor_annotation.py.
1 participant
Summary
qd.Tensorkernel-argument slot now accepts external arrays (numpy.ndarray,torch.Tensor), routed through the same ndarray feature path as a bareqd.ndarrayand keyed by dtype/rank. Previously torch produced one specialization per tensor instance and numpy raisedTypeError: unhashable type: 'numpy.ndarray'..shape/.dtypecheck: those are exactly the external types the launch path (FuncBase._recursive_set_args) can bind, and a duck-typed check would wrongly divert quadrants-nativeField/SNode(which also expose.shape/.dtype) off the template path._template_mapper_hotpath._extract_arg) and the launch path (_func_base).tensor.mddocuments the accepted types.This matches the set of external arrays a
qd.types.ndarray()slot already accepts, soqd.Tensorandqd.types.NDArraynow support the same external types end-to-end. Independent of theNonehalf of #856 (W1/W2/W4/W5) and of #831.Test plan
test_tensor_annotation.py: numpy and torch each collapse to a single specialization (not one per instance); numpy no longer raisesunhashable; aFieldthrough the sameqd.Tensorslot still takes the template path.test_tensor_annotation.pyfull file: 30 passed on cpu, from a clean branch checkout.run_tests.py --arch cpu): 4340 passed, 4 failures that are pre-existing (verified identical on cleanmain: 3xtest_committed_fatbin_layoutunder a CUDA-OFF build, 1xtest_pyi_stubs) - 0 regressions from this change.Built CPU-only (
QD_WITH_CUDA=OFF) on the cluster; W3 is backend-agnostic Python dispatch. A perf-tuned fast path for the predicate on the hotqd.Tensortemplate branch can be layered later if profiling warrants.Made with Cursor