Skip to content

Copy domains list when creating kernels in realize_reduction#1032

Merged
inducer merged 1 commit into
inducer:mainfrom
majosm:realize-reduction-copy-domains
Jul 24, 2026
Merged

Copy domains list when creating kernels in realize_reduction#1032
inducer merged 1 commit into
inducer:mainfrom
majosm:realize-reduction-copy-domains

Conversation

@majosm

@majosm majosm commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Avoids accidental mutation of kernel.domains from sharing of domains list reference between kernel and red_realize_ctx.

@inducer
inducer force-pushed the realize-reduction-copy-domains branch from 9ac506a to 473d029 Compare July 24, 2026 17:06
@alexfikl

Copy link
Copy Markdown
Contributor

I think I touched some of this last year when doing some typing changes. Wasn't the idea to move more towards making these immutable? i.e. domains are a Sequence so that they can accept a tuple?

@inducer

inducer commented Jul 24, 2026

Copy link
Copy Markdown
Owner

I think I touched some of this last year when doing some typing changes. Wasn't the idea to move more towards making these immutable? i.e. domains are a Sequence so that they can accept a tuple?

That'd be nice. I suppose typing could help enforce this. (Though we'd probably have to tighten the type to tuple, at least briefly for development.)

@alexfikl

Copy link
Copy Markdown
Contributor

That'd be nice. I suppose typing could help enforce this. (Though we'd probably have to tighten the type to tuple, at least briefly for development.)

Also that would be a pretty breaking change, in case any downstreams expect it to be mutable. Anyways, that's a bigger change, maybe not for this PR 😁

@inducer

inducer commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Also that would be a pretty breaking change, in case any downstreams expect it to be mutable. Anyways, that's a bigger change, maybe not for this PR 😁

Not really! The data structure was always notionally immutable, just not factually so.

@inducer
inducer merged commit 5c281bd into inducer:main Jul 24, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants