feat: Use array format for index-set sequences - #175
Conversation
|
I'll take a look at this once I find some free time. Some observations from a quick skim: Introducing index set mutationI might be mistaken, but it looks like this PR makes two simultaneous, but orthogonal changes:
I'm somewhat surprised that (a) didn't break tests, as mutation should be less conservative than allocating sets. Maybe this is a perk of the Jaxpr language being functional. Maybe our test set doesn't yet cover such edge cases. Maybe I also just didn't skim the code rigorously enough and (a) is somehow enabled by (b). If this change is in orthogonal to (b), its performance gains should be benchmarked separately from the set types. New set types
Could you explain this format to me on a simple example? You introduce a large amount of new types and abstractions: e.g, Side note for @gdalle: you will be delighted to see your beloved |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
==========================================
+ Coverage 93.84% 93.86% +0.01%
==========================================
Files 61 61
Lines 3999 4191 +192
==========================================
+ Hits 3753 3934 +181
- Misses 246 257 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
I think (a) is indeed an orthogonal change, that was necessitated by the new objects interacting with regular sets. I think perhaps now it is not needed anymore. The non-mutating alternative would be I think it is indeed just a coincidence that it doesn't break anything, but then again I don't see many situations in which you would use
Of course, I can split it off into a separate commit, or if it's not needed anymore I can also remove the For performance testing, should I include this type of "large constant" calculation test? Or do you have a different suggestion? And how/when should it be run? Every pytest run seems excessive.
To clarify, For the separate types:
Is that clearer? It's a lot of types but I don't really see a way in which removing any of them or folding them into one would make the code simpler, it would only create objects with mixed responsibilities (IMO). |
|
I've removed the |
|
Thanks for the explanations!
Why do we need both COO and CSR? Where is which one used?
If 3 out of 6 types (
Shouldn't all index sets be views then? When are views used over allocating new sets and vice-versa? If this is just used in intermediate computations within handlers, we might want to make this a class method of
As I've mentioned in #174, I have large concurrent branch of work I did with Fable two weeks ago, so this won't be a simple squash merge. I just made it public as a draft PR in #177 in case you are curious (it's a very large refactor and I haven't had time to review it yet either). Since I'm a strapped for time (I need to write up my PhD thesis this summer), I'll prioritize reviewing and merging #176 first. This PR is also missing benchmarks demonstrating improvements on both small and large problems (~100k-1M inputs). I worry
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the interpreter’s internal “index-set sequence” representation (list[set[int]]) with a compact, array-backed CSR-like format (IndexSetSequence) plus a builder (IndexSetSequenceBuilder) to reduce memory/time overhead when tracking sparsity for very large arrays.
Changes:
- Introduces
IndexSetSequence/IndexSetView/IndexSetSequenceBuilderand updatesStateIndicesto normalize assigned values intoIndexSetSequence. - Updates multiple propagation rules to use views/builders (
|=) instead of mutating or constructing per-element Python sets/lists. - Adds a dedicated unit test suite for the new containers and adjusts existing tests for the new return types.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/_interpret/test_internals.py | Updates assertions to accommodate IndexSetSequence element views. |
| tests/_interpret/test_index_set_sequence.py | Adds comprehensive unit tests for IndexSetSequence, views, builder semantics, and StateIndices normalization. |
| src/asdex/detection/_interpret/CLAUDE.md | Updates internal interpreter docs to describe the new containers and aliasing/immutability rules. |
| src/asdex/detection/_interpret/_while.py | Ensures while-loop carry is copied into mutable sets before fixed-point mutation; updates typing/contracts. |
| src/asdex/detection/_interpret/_stack.py | Switches pooled indices container to store IndexSetView rather than mutable sets. |
| src/asdex/detection/_interpret/_sort.py | Replaces per-element assignment with builder ` |
| src/asdex/detection/_interpret/_scatter.py | Adjusts scatter handler signatures/outputs to work with IndexSetView sequences. |
| src/asdex/detection/_interpret/_scan.py | Threads carry as sequences of set-likes and concatenates per-step outputs using views. |
| src/asdex/detection/_interpret/_reduce.py | Uses IndexSetSequenceBuilder for reduction accumulation via ` |
| src/asdex/detection/_interpret/_pad.py | Adjusts output typing to allow returning set-like views/sets without assuming mutability. |
| src/asdex/detection/_interpret/_gather.py | Updates gather enumeration callback types to return view-based sequences. |
| src/asdex/detection/_interpret/_elementwise.py | Returns views directly when one derivative is globally zero; avoids unnecessary copies. |
| src/asdex/detection/_interpret/_dynamic_slice.py | Switches dynamic slice/update to use view replacement rather than deep-copying/mutating sets. |
| src/asdex/detection/_interpret/_dot_general.py | Uses builder for output accumulation instead of pre-allocating a list of sets. |
| src/asdex/detection/_interpret/_cond.py | Uses builder-based copying/merging for branch output unions. |
| src/asdex/detection/_interpret/_concatenate.py | Switches pooled indices container to store IndexSetView. |
| src/asdex/detection/_interpret/_common.py | Implements core array formats, view/sequence/builder types, updates StateIndices, and adapts common helpers to set-like sequences. |
| src/asdex/detection/_interpret/init.py | Updates _prop_jaxpr signature/return types and internal state initialization to use StateIndices. |
| src/asdex/detection/_api.py | Updates input seeding to build IndexSetSequence efficiently (identity/empty builders). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| start = self.set_offsets[index] | ||
| stop = self.set_offsets[index + 1] | ||
| return IndexSetView(self.int_indices[start:stop]) |
| selected_starts = self.set_offsets[index] | ||
| selected_stops = self.set_offsets[index + 1] | ||
| selected_lengths = selected_stops - selected_starts |
| # Scalar target: normalize this one element's members to a 1-D int32 | ||
| # array now, keeping the index itself scalar (expanded at build). | ||
| self._scalar_writes.append((index, self._scalar_members_to_array(value))) | ||
|
|
| ``target_indices.size``. The kind of ``value`` selects its container. | ||
| Typed ``object`` so the ``isinstance`` ladder narrows each arm cleanly. | ||
| """ | ||
| if isinstance(value, IndexSetSequence): |
Because COO is efficient when building the sets, and CSR when reading them later. I feel like I am repeating what I said before, is there something still unclear with regards to these structures?
Okay, so you'd rather I update all set-building code to work with a new interface? I thought it would be preferable to keep the changes as localized as possible, but that works too :)
When reading yes, you are almost always using a view. Building a new set-sequence is the only situation in which you need to take a number of these views and recombine them in a different manner.
What are you referring to here?
I meant, my new changes after your first comments can be squashed into my initial changes, since they make the scope of the MR smaller :)
Sure, performance benchmarks would be good to add. I asked before if you have any opinion on what would be a representative benchmark, otherwise I can just add this "large constant" test. However with regard to your set-type comment, I don't think there would be any situation where |
Summary
This PR adds a new format for storing index-set sequences during detection, which previously were stored as
list[set[int]]. These are collections of sets, that are linked to a certain value involved in the traced computation. There is one set per element of the value, and each set contains the indices of the elements in the input that it depends on.Storing these as
list[set[int]]can become quite slow when large arrays with millions of elements are involved. In this case, the representation is also not very sparse: though there may only be few dependencies, even if there are no dependencies we still have to create millions of empty sets. This can become a memory and performance bottleneck.The new format does two things:
list[set[int]]-like API, the index-set sequences can also be indexed in a vectorized manner (asiss[arr]) and the builder can perform vectorized union operations (asbuilder[arr1] |= arr2). These also integrate with each other (builder[out_arr] |= iss[in_arr]), which is a common pattern since input dependencies are often propagated as-is.Further work would be to modify propagation rules to build the index-set sequences in a vectorized manner, supported by the new vectorized API.
There are no public API or otherwise user-facing changes in this PR.
Related issue
Closes #174.
Checklist
uv run prek run --all-files).uv run pytest).CHANGELOG.mdare updated for user-facing changes.