feat[next]: Add support for tuple comprehensions - #2833
Conversation
| case ts.VarArgType(element_type=element_type): | ||
| new_type = ( | ||
| element_type # TODO: we only temporarily allow any index for vararg types | ||
| ) |
There was a problem hiding this comment.
Comment from @tehrengruber in #2487:
This is for direct access to tracers[0] * factor, tracers[1] * factor, which I personally think is an anti pattern. I left it here until we take a decision on this. We could also make it an optional feature. One of the disadvantages is that it is not possible to fully type check the field operator at definition time, since the tuple length is only known at call / compile time. The user will then get an error in unroll_map_tuple.
|
|
||
| def visit_Name(self, node: ast.Name, **kwargs: Any) -> foast.Name: | ||
| return foast.Name(id=node.id, location=self.get_location(node)) | ||
| def visit_Name(self, node: ast.Name, **kwargs: Any) -> foast.DataSymbol | foast.Name: |
There was a problem hiding this comment.
It made 'visit_Name' produce a 'DataSymbol' for names in 'ast.Store' context, which the comprehension-target parsing ('parse_target') relied on when visiting the loop target; everything else only ever visits names in 'Load' context. Since 'parse_target' was the only user, 374327f (pending push) builds the 'DataSymbol' directly there and reverts 'visit_Name' to main's version.
| iterable_element: itir.Expr | str, | ||
| ) -> itir.Expr: | ||
| """ | ||
| Wrap `element_expr` in a `let` binding the comprehension target to `iterable_element`. |
There was a problem hiding this comment.
| Wrap `element_expr` in a `let` binding the comprehension target to `iterable_element`. | |
| Wrap `element_expr` in a `let` binding the `comprehension_target` to `iterable_element`. |
There was a problem hiding this comment.
Applied in 410f729a5 (pending push).
|
|
||
| returns `let a = iterable_element[0], b = iterable_element[1] in element_expr`. |
There was a problem hiding this comment.
| returns `let a = iterable_element[0], b = iterable_element[1] in element_expr`. | |
| returns | |
| `let a = iterable_element[0], b = iterable_element[1] in element_expr`. |
There was a problem hiding this comment.
Applied in 410f729a5 (pending push).
| tuple_types = [from_type_hint_same_ns(arg) for arg in args] | ||
| assert all(isinstance(elem, ts.DataType) for elem in tuple_types) | ||
| return ts.TupleType(types=tuple_types) | ||
| if ( |
There was a problem hiding this comment.
@tehrengruber-ai Add a comment with short annotation examples for all the 3 cases here.
There was a problem hiding this comment.
Done in 410f729a5 (pending push): one short example comment per branch — fixed-length 'tuple[int32, float64]', variable-length 'tuple[int32, ...]', and unparametrized 'tuple' / 'tuple[()]'.
| case common.Field: | ||
| if (n_args := len(args)) != 2: | ||
| raise ValueError(f"Field type requires two arguments, got {n_args}: '{type_hint}'.") | ||
| if args is None or len(args) != 2: |
There was a problem hiding this comment.
It adapts the 'Field' case to the change above in 'canonicalize_type_hint', which now returns 'args=None' for unsubscripted annotations (needed to distinguish 'tuple' from 'tuple[()]'). A bare 'Field' annotation thus arrives here with 'args=None', and main's 'len(args)' would raise a 'TypeError'; the guard preserves the original diagnostic (verified: bare 'Field' still raises ValueError "Field type requires two arguments, got 0"). Added a short comment saying so in 410f729a5 (pending push).
Allow indexing 'tuple[Field, ...]' parameters directly, e.g. 'tracers[0] * factor': the subscript type deduction accepts any literal index on a 'ts.VarArgType' and yields the element type. Extracted from #2833 pending a decision; likely to be skipped, since the field operator cannot be fully type checked at definition time (the tuple length is only known at call/compile time, so an out-of-range index only surfaces late, in 'unroll_map_tuple').
5b27cbe to
35bce19
Compare
tehrengruber-ai
left a comment
There was a problem hiding this comment.
Automated review by Fable (posted on behalf of @tehrengruber). Six findings, each verified empirically by comparing pre-PR vs. post-PR behavior. No issues found in the core comprehension typing/lowering itself; everything clusters around the variadic-tuple plumbing at the edges (out= validation, builtin-constructor arity, the DeferredType stand-in for bare tuple, dispatch, and test-harness coverage).
| if operator_return_type != new_kwargs["out"].type: | ||
| # Not equality: variadic comprehensions give a `VarArg[...]` return type, | ||
| # while 'out' is a concrete tuple. | ||
| if not type_info.is_compatible_type(operator_return_type, new_kwargs["out"].type): |
There was a problem hiding this comment.
[correctness, verified] Loosening this from equality to is_compatible_type/is_concretizable for all operators silently drops tuple-length validation for VarArg-returning operators: the return type is never specialized to the call-site length, and is_concretizable(VarArg[T], Tuple) ignores length (it even accepts an empty tuple, see type_info.py:555).
Verified: calling a tuple[IField, ...] -> tuple[IField, ...] operator with a 3-element input and a 2-element out passes PAST type deduction; embedded execution then dies with a bare AssertionError in utils.tree_map (utils.py:211), roundtrip with the opaque 'target_domain' cannot be 'NEVER' error. Pre-PR the equality check gave a located "Expected keyword argument out to be of type ..." error.
Suggestion: specialize the VarArg return type against the concrete argument length (available in with_args) instead of weakening the check for every operator. Note foast_to_past.py:121 uses a different predicate (is_concretizable) for the same conceptual check.
There was a problem hiding this comment.
This is a complicated issue that requires in-depth investigation. One solution would be integer type parameters that are resolved later with a proper frontend message or moving type checking to GTIR completely with a way to translate source locations. Skipped for now.
@tehrengruber-ai Write a test that shows this. Maybe pass two var args tuples of different length and conditionally return either one of them.
There was a problem hiding this comment.
Test added in ed23528 (pending push): 'test_var_len_tuple_length_mismatch_rejected' in test_tuples.py uses your construction — 'a: tuple[IField, ...]' of length 2, 'b' of length 3, 'return a if c else b', 'out' of length 2 — so no single length can be consistent with 'out'. It asserts the desired located 'DSLError' and is marked xfail(strict=True) documenting the gap: today every backend dies with an internal error instead (embedded and roundtrip with a bare 'AssertionError' from 'utils.tree_map', gtfn with a bare 'AssertionError' at compile). Verified XFAIL on all 7 runnable matrix variants, no XPASS.
There was a problem hiding this comment.
Related but separate half, in my review body above: _is_generic does not recognise VarArgType, so tuple length never enters the compiled-program cache key either. #2834 fixes that predicate, but compile() never passes arg_specialization_info, so the fixed predicate makes variadic-tuple programs un-AOT-compilable. Not covered by the xfail test here.
| f"'{self._func_name(node)}()' only takes literal arguments.", | ||
| ) | ||
| assert isinstance(node.func, ast.Name) | ||
| (arg,) = node.args |
There was a problem hiding this comment.
[correctness, verified] The new (arg,) = node.args turns wrong-arity builtin constructor calls into raw unpacking errors, regressing the located diagnostic: pre-PR, int32() or int32(1, 2) in a field operator raised a located DSLError: Invalid argument types in call to 'int32'; post-PR both crash with a bare ValueError: not enough/too many values to unpack and no source location. The old if len(node.args) > 0 tolerated non-1 arity and let type deduction report it properly.
There was a problem hiding this comment.
Fixed in ed23528 (pending push): wrong arity now raises a located DSLError ("'int32()' takes exactly one argument, got 2.") before the unpack, pinned by 'test_scalar_cast_wrong_arity' for both the 0- and 2-argument case. Same wording as the arity check on the element-wise branch, so the two merge cleanly.
| ) | ||
| ) | ||
|
|
||
| case ts.DeferredType(constraint=ts.TupleType): |
There was a problem hiding this comment.
[correctness, verified] This constructor case returns DeferredType(constraint=VarArgType), but no call path can ever concretize it: the genexp path bypasses it in the parser, and the still-permitted literal path trips an internal assert. Verified: tuple(1.0) inside a field operator (accepted by _verify_builtin_type_constructor, whose own error message offers "literal arguments or a generator expression") fails with AssertionError: 'FOAST' expression is not fully typed.
Either reject tuple(<literal>) in the parser alongside other non-genexp forms, or make the constructor produce a concrete type.
There was a problem hiding this comment.
Traced it: this branch types the 'tuple' builtin symbol itself, not user annotations. 'tuple' is in 'fbuiltins.PYTHON_TYPE_BUILTINS', so when a field operator references the name outside the intercepted genexp form, the symbol is typed via 'from_value(tuple)' -> 'type[tuple]' -> 'make_constructor_type', which lands here; without the branch 'make_constructor_type' falls through to its closing ValueError and parsing crashes while typing the symbol. The genexp form never needs it — verified: a parsed 'tuple(genexp)' operator has no 'tuple' closure symbol at all, since the parser intercepts the call before name resolution. The DeferredType return value is only ever consumed on the 'tuple()' path, which then dead-ends in the not-fully-typed assert — that path should be rejected in '_verify_builtin_type_constructor' so the return type is never consulted.
| elif args is None or (isinstance(args, tuple) and len(args) == 0): | ||
| # TODO(tehrengruber): We use `DeferredType` until we have an actual representation | ||
| # for a generic type. | ||
| return ts.DeferredType(constraint=ts.TupleType) |
There was a problem hiding this comment.
[correctness, verified] A bare tuple annotation now maps silently to this DeferredType stand-in instead of being rejected here, so the rejection surfaces downstream as an internal-repr message: a parameter annotated bare tuple fails at decoration with InvalidParameterAnnotationError: ... 'DeferredType(constraint=<class ...TupleType>)' (raw datamodel repr shown to the user), where pre-PR it was the clear ValueError: Tuple annotation 'tuple' requires at least one argument. Any consumer of from_type_hint not guarding against DeferredType inherits the same leak.
There was a problem hiding this comment.
Same trace from the 'from_type_hint' side: 'from_value(tuple)' goes through the 'type[Foo]' case ('type_translation.py:300'), which calls 'from_type_hint(tuple)' for the constructed type — so bare 'tuple' must map to something, and this branch supplies the 'DeferredType(constraint=TupleType)' stand-in (per the TODO, until a real generic-tuple type exists). The side effect is that bare 'tuple' annotations share the code path, which is why they now surface downstream as the 'InvalidParameterAnnotationError' with a raw DeferredType repr instead of main's clear "requires at least one argument" ValueError. If wanted, the annotation entry point could reject DeferredType with a proper message while keeping this branch for the builtin-symbol use.
There was a problem hiding this comment.
Adjacent: the comment at line 191 still says the None/() split exists to distinguish tuple from tuple[()], but this branch maps both to the same DeferredType. Commented there.
| if isinstance(node.func, ast.Name): | ||
| func_name = self._func_name(node) | ||
|
|
||
| if ( |
There was a problem hiding this comment.
[design, verified] Comprehension recognition is ~50 lines inlined in visit_Call, keyed purely on the syntactic name tuple, ahead of and outside the dialect's builtin-constructor dispatch (TYPE_BUILTIN_NAMES below), with no symbol resolution. Aliasing (t = tuple at module scope, then t(x * f for x in tracers)) falls through to the generic UnsupportedPythonFeatureError whose hint says generator expressions "are only supported as the argument of tuple(...)" - exactly what the user semantically wrote; conversely a closure variable that merely happens to be named tuple is rerouted into comprehension parsing. Extracting a _visit_tuple_comprehension (or using the builtin-constructor table) and resolving the name against closure vars would make the dispatch robust.
There was a problem hiding this comment.
This is a general problem that we want to solve, out-of-scope in this PR. One idea is to introduce a "type of types" like type[int] and use this to propagate type information.
| _allocate_from_type( | ||
| case=case, arg_type=t, domain=domain, dtype=dtype, strategy=strategy | ||
| ) | ||
| for t in [element_type] * 3 # TODO: revisit |
There was a problem hiding this comment.
[test-coverage, verified] The harness hardcodes exactly 3 elements for every VarArgType allocation (and again in get_param_size, both marked TODO), so no integration test can exercise any other variadic length. That makes length-dependent bugs structurally invisible - including the missing out-length check flagged in past_passes/type_deduction.py on this PR. Suggestion: make it an explicit, overridable harness parameter (e.g. a per-case default_varargs_length).
There was a problem hiding this comment.
The user can always allocate a tuple of the size he likes so this doesn't feel like an issue.
There was a problem hiding this comment.
Let's add a comment and then remove or add a message to the todo.
There was a problem hiding this comment.
And extract the magic number into a named object.
…-mismatch xfail test
havogt
left a comment
There was a problem hiding this comment.
Review of the current head (ed23528a). Everything below was checked by running against the branch unless I say otherwise.
Two findings have no anchor in the diff, because the files they concern are not touched by this PR. They are the two most important ones, so they go here.
1. Module-level names are unreachable from inside a comprehension body
This is a functional blocker for the feature, not a rough edge.
@gtx.field_operator
def testee(tracers: tuple[EField, ...]) -> tuple[CField, ...]:
return tuple(neighbor_sum(t(C2E), axis=C2EDim) for t in tracers)
# UndefinedSymbolError: Undeclared symbol 'neighbor_sum'neighbor_sum is imported at module scope and works one line earlier outside a comprehension.
Cause. A generator expression compiles to its own code object, so names referenced only inside it land in that object's co_names, never in the enclosing function's. ffront/source_utils.py:24 get_closure_vars_from_function uses inspect.getclosurevars, which inspects only the enclosing function:
outer co_names : ('tuple',)
nested <genexpr> co_names : ('neighbor_sum', 'C2E', 'C2EDim')
collected closure vars : ['tuple']
Scope. Every module-level name: gt4py builtins (neighbor_sum, where, astype, ...), every FieldOffset, every local Dimension, and module-level @field_operators — so the clean factoring tuple(_inner(t, ...) for t in tracers) does not work either. Free variables are unaffected, because the enclosing code object carries co_freevars for the nested one.
Why CI is green. Every comprehension test in test_tuples.py defines its helpers inside the test function, so they are free variables. test_tuple_comprehension_other_fo passes for that reason alone. The entire suite sits on the working side of the bug.
Two things make this worse than a plain limitation: the diagnostic names a symbol that is plainly imported, and the workaround is to also reference the name outside the comprehension — so whether a stencil compiles depends on an unrelated line elsewhere in the function.
Prototype fix. Offered as a starting point, not a finished patch — it has not been reviewed by anyone but me, and the notes below are things I would want settled before it landed:
def _iter_code_objects(code: types.CodeType) -> Iterator[types.CodeType]:
yield code
for const in code.co_consts:
if isinstance(const, types.CodeType):
yield from _iter_code_objects(const)
def _global_vars_from_nested_code(function: Callable) -> tuple[dict[str, Any], dict[str, Any]]:
global_ns = function.__globals__
builtin_ns = global_ns.get("__builtins__", builtins.__dict__)
if inspect.ismodule(builtin_ns):
builtin_ns = builtin_ns.__dict__
global_vars: dict[str, Any] = {}
builtin_vars: dict[str, Any] = {}
for nested_code in _iter_code_objects(function.__code__):
if nested_code is function.__code__:
continue
for name in nested_code.co_names:
if name in global_ns:
global_vars[name] = global_ns[name]
elif name in builtin_ns:
builtin_vars[name] = builtin_ns[name]
return global_vars, builtin_vars
def get_closure_vars_from_function(function: Callable) -> dict[str, Any]:
(nonlocals, globals, builtins_, _unbound) = inspect.getclosurevars(function)
nested_globals, nested_builtins = _global_vars_from_nested_code(function)
return dict(sorted(
{**nested_builtins, **builtins_, **nested_globals, **globals, **nonlocals}.items()
))Resolution order mirrors inspect.getclosurevars itself; nonlocals still win. The comprehension target is not collected, because it is in the nested object's co_varnames rather than co_names.
Results on this branch: tests/next_tests/unit_tests/ 2144 passed, 51 skipped, 14 xfailed — no regressions. The comprehension tests in test_tuples.py pass on gtfn CPU including skip_value_mesh. mypy src/ and pre-commit are clean.
Caveats I would want settled: _iter_code_objects yields the root only for its caller to discard it; the merge ordering is defensive rather than meaningful, since both sources resolve from the same namespaces; and I have not checked the bound-method path, where getclosurevars unwraps to __func__ but this helper does not. It also does not address gtx.astype(...) inside a comprehension — but that fails outside a comprehension too, with DSLError: Functions can only be called directly, so it is a separate pre-existing restriction and not part of this bug.
I have not opened a PR for this; it is yours to take or discard.
2. Variable-length tuple programs are not treated as generic
CompiledProgramsPool._is_generic (otf/compiled_program.py) tests only ts.DeferredType. A program taking tuple[Field, ...] has VarArgType argument types, so it is classed non-generic and the tuple length never enters the compiled-program cache key — two calls with different tuple lengths reuse one compiled variant. On this head:
program arg types: ['VarArg[Field[[IDim], float64]]', 'float64', 'VarArg[Field[[IDim], float64]]']
any DeferredType (the rule used here): False
#2834 fixes the predicate by recursing into VarArgType. Please do not simply move that fix down here, though: compile() never passes arg_specialization_info, and _compile_variant then raises "Can not precompile generic program or scan operator without argument types." for generic programs. So the #2834 predicate makes ahead-of-time compilation of variadic-tuple programs impossible, and there is no parameter on the public compile() through which to supply the length. That raise pre-exists on main, where it already blocks AOT for scan operators, so this is a gap in the AOT API surface rather than a defect introduced here — but it means fixing the cache key alone converts a silent wrong-result into a hard block. Worth deciding deliberately.
The new test_var_len_tuple_length_mismatch_rejected xfail acknowledges an adjacent symptom; this is the other half.
Smaller points inline below.
| return self.visit(target_el, refine_type=type_, **kwargs) | ||
| except IndexError: | ||
| raise errors.DSLError( | ||
| target_el.location, f"Cannot unpack non-iterable '{type_}' object." |
There was a problem hiding this comment.
Two issues with this handler.
First, the message conflates two different failures. An IndexError here means either the element is not a tuple at all, or it is a tuple that is too short for the requested unpacking pattern. Both report "Cannot unpack non-iterable ... object", which is wrong for the arity-mismatch case. (Raised on the predecessor PR as #2487 (comment) 3318935481.)
Second, and newly introduced by this formulation: the try block wraps self.visit(target_el, refine_type=type_, **kwargs) on line 769, not just the path walk above it. Any IndexError raised anywhere inside that visit — including one from an unrelated bug — is caught and reported as an unpacking failure, with type_ holding the fully resolved leaf type by then, which makes the message nonsense. Narrowing the try to the for loop would fix both the masking and half the message ambiguity.
There was a problem hiding this comment.
@tehrengruber-ai agreed. Make sure there is a type_deduction test for the two errors.
There was a problem hiding this comment.
Fixed in 4f24c6d (pending push). The two failure modes now raise distinct located DSLErrors — "Cannot unpack non-iterable '' object." for a non-tuple element and "Not enough values to unpack (expected at least N, got M)." for a too-short tuple — and the 'try' wrapper is gone entirely (the checks raise directly, so an unrelated IndexError from 'self.visit' can no longer be swallowed and misreported). Both errors are pinned by new type_deduction tests: 'test_tuple_comprehension_unpack_non_tuple' and 'test_tuple_comprehension_unpack_too_short_tuple'.
| element_types = cast(list[ts.DataType], iterable.type.types) | ||
| # Only homogeneous iterables are supported, see ADR 0028. | ||
| if not all(element_type == element_types[0] for element_type in element_types): | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
NotImplementedError rather than errors.DSLError for what is a user-facing restriction: the user gets a bare traceback with no source location pointing at their comprehension.
ADR 0028 is a good writeup of why the restriction exists, and the "(see ADR 0028)" reference is helpful — but the diagnostic should still be able to point at the offending line. Every other rejection in visit_TupleComprehension (empty tuple, non-data element types, non-tuple iterable) already raises DSLError with iterable.location.
There was a problem hiding this comment.
@tehrengruber-ai Use a plain DSLError mentioning that this is not implemented.
There was a problem hiding this comment.
Done in 4f24c6d (pending push): now a plain located DSLError at 'iterable.location' — "Tuple comprehensions over fixed-length tuples with differently typed iterable elements are not implemented (see ADR 0028)." The three rejection tests in test_foast_to_gtir.py updated accordingly.
|
|
||
| canonical_type = typing.get_origin(type_hint) or type_hint | ||
| args = typing.get_args(type_hint) | ||
| # In order to distinguish `tuple` from `tuple[()]`, the former returns None here. |
There was a problem hiding this comment.
This comment and the one at line 225 now contradict each other.
Here: "In order to distinguish tuple from tuple[()], the former returns None here." At line 225: "Unparametrized annotation, i.e. tuple or tuple[()]" — and the branch maps both to the same ts.DeferredType(constraint=ts.TupleType). I confirmed they are identical in output:
tuple[()] -> DeferredType(constraint=TupleType)
tuple -> DeferredType(constraint=TupleType)
So the None-versus-() machinery is built and then deliberately not used. That is a defensible choice — but as written, the first comment tells a reader the distinction is load-bearing when it is not. Either drop this comment, or keep the distinction and map tuple[()] to ts.TupleType(types=[]), which is what the annotation actually means.
(Note tuple[()] raised ValueError on main, so nothing regresses either way — it is only a question of which behaviour is intended.)
There was a problem hiding this comment.
@tehrengruber-ai Why is it that way? The unparameterize tuple should just return None.
There was a problem hiding this comment.
It was that way because 'get_args' cannot distinguish the cases: bare 'typing.Tuple' also has origin 'tuple' and 'get_args() == ()', identical to 'tuple[()]' — so the branch lumped '()' in with 'None' to keep 'typing.Tuple' working, leaving the None-machinery unused as Hannes observed. Fixed in 4f24c6d (pending push): 'canonicalize_type_hint' now keys on the 'args' attribute (unsubscripted typing aliases have a truthy origin but no 'args'), so 'None' really means unparametrized for every spelling ('tuple', 'typing.Tuple') while the empty-tuple annotation ('tuple[()]', 'typing.Tuple[()]') keeps 'args == ()' and is rejected with the existing "Tuple annotation ..." ValueError. Comments on both sides updated; pinned in test_type_translation.py.
| if isinstance(symbol_type, ts.VarArgType) and isinstance(to_type, ts.VarArgType): | ||
| return is_concretizable(symbol_type.element_type, to_type.element_type) | ||
| if isinstance(symbol_type, ts.VarArgType) and isinstance(to_type, ts.TupleType): | ||
| if len(to_type.types) == 0 or ( |
There was a problem hiding this comment.
len(to_type.types) == 0 short-circuits to True unconditionally, so a VarArgType is considered concretizable to the empty tuple regardless of its element type — e.g. a field operator returning tuple[Field, ...] would accept out=().
Probably harmless in practice given where this is called from, but it is an unguarded hole rather than a deliberate identity case. If the empty tuple genuinely should be accepted, a short comment saying why would help; otherwise it wants the same element check as the non-empty path.
There was a problem hiding this comment.
@tehrengruber-ai "so a VarArgType is considered concretizable to the empty tuple regardless of its element type" is sound. That a field operator accepts an empty tuple for a vararg can also make sense, add a test. If fixing it is complicated let me know.
There was a problem hiding this comment.
Test added in 4f24c6d (pending push): 'test_is_concretizable_vararg_to_tuple' in test_type_info.py pins the empty tuple as a valid concretization (vacuously true — no elements whose type could conflict), alongside matching/mismatching non-empty cases; also added the short why-comment at the check itself. On the end-to-end side, letting you know per your offer: an actual empty-tuple call works in embedded ('scale((), 2.0, out=())' runs fine) but the compiled path crashes in 'past_to_itir._visit_stencil_call_out_arg' — an empty 'out' tuple leaves the SetAt with nothing to unpack ('ValueError: not enough values to unpack (expected 2, got 0)'). Supporting it there means deciding what a SetAt with an empty target does (probably: emit no SetAt at all) across past_to_itir and domain inference, so I left the compiled path alone and did not add a matrix-wide integration test.
|
@tehrengruber-ai Some directions:
|
…ty, tuple[()] handling, empty-vararg test)
|
Both directions addressed: 1. The test for the example already exists — 'test_var_len_tuple_length_mismatch_rejected' in test_tuples.py (ed23528) uses exactly the conditional-return construction and asserts the desired located DSLError as xfail(strict). The proposed issue text is drafted in 'submit_vararg_length_issue.sh' in the worktree: it describes the missing length validation, both the plain out-mismatch and the ambiguous conditional-return example, the specialization-in-'with_args' direction with the dedicated-field-operator workaround, and the 'foast_to_past' vs PAST-deduction predicate discrepancy. 2. Done in e449e6982 (pending push): 'test_var_len_tuple_comprehension_different_lengths' calls the same operator with lengths 3 then 2 — before the fix the second call reused the length-3 compiled variant and crashed with 'IndexError: tuple index out of range'. '_is_generic' now treats parameters containing a 'VarArgType' (also nested inside tuples) as generic, so each length gets its own specialized variant; verified across the backend matrix and the otf unit tests. The compile gap for the empty tuple stays out, as decided — xfailed test plus issue draft 'submit_empty_tuple_issue.sh'. |
|
I think you @tehrengruber misunderstand the severity of 1 (or I misunderstood the comment). Also module level field_operators have this problem. |
`get_closure_vars_from_function` used `inspect.getclosurevars`, which reads only the enclosing function's code object. A generator expression compiles to its own code object, so names referenced only inside a tuple comprehension body — gt4py builtins, `FieldOffset`s, `Dimension`s, module-level field operators — were never collected, and the comprehension failed with `UndefinedSymbolError` on a name that is visibly imported. Free variables were unaffected, which is why every comprehension test in GridTools#2833 passes: they define their helpers inside the test function. Prototype for gt4py-f24. Not proposed upstream; committed so the reproducer and fix outlive the worktree they were written in. Claude-Session: https://claude.ai/code/session_01F8vDQpsHU76tFgD1tk4GAC
Adds support for tuple comprehensions, e.g. for usage on tracers.
TODO:
AI disclaimer: This code was to a small degree refactored using AI tools. Code has been reviewed in detail, tests only briefly.