Repository navigation
bindings/python: +feat Import transitions in bulk and document wrapper ownership - #892
Merged
Merged
Conversation
Adda0
force-pushed
the
issue-796-python-bulk-pr
branch
from
October 15, 2026 12:14
d24bbf0 to
c160bf5
Compare
Adda0
marked this pull request as ready for review
October 15, 2026 12:57
…r ownership Building an automaton from Python went one transition at a time through Nfa.add_transition or Delta.add: one Python call, one argument conversion and one sorted insertion per transition. Delta gains two bulk entry points over the C++ batched import Delta::add(std::vector<Transition>&&): add_transitions(sources, symbols, targets) taking three equal-length sequences, and add_transitions_from(triples) taking an iterable of (source, symbol, target). Both convert and range-check the whole input into C++ vectors before touching the delta, so invalid input raises and leaves the automaton exactly as it was, and both release the GIL around the native sort and build. The semantics match repeated add(): duplicates collapse, states up to the largest mentioned one are allocated, and an import into a non-empty delta merges. Measured in this workspace, 200000 random transitions over 50000 states: per-transition add_transition 120.5 ms, add_transitions 59.5 ms, a 2.02x speedup, both producing identical automata. Peak RSS grows by 66 MB for the per-transition loop and by 72 MB for the bulk import, which additionally holds the three input sequences and the converted vector at the same time, so the bulk path trades a little peak memory for half the time. The two "TODO: Refactor into a generator" are resolved: iter_transitions_from(state) streams the post of one state by reference, get_trans_from_state_as_sequence() is a list of it, and iterate() is documented as the streaming form of get_trans_as_sequence(). Return types are unchanged. The ownership rules are now written down in the .pxd and .pyx docstrings and each has a test: Nfa and Nft own the C++ automaton through a shared_ptr; a Delta view holds an aliasing shared_ptr and keeps its automaton alive; the iteration generators keep their owner alive; Transition, SymbolPost and Run own a raw heap object. The shared_ptr TODO in nfa.pxd is resolved by measurement rather than deleted blindly: creating and dropping a Delta view costs 128 ns and a cheap method call through thisptr.get() costs 82 ns, so the atomic reference count is not visible next to the Python call overhead itself. The TODO is replaced by that number. Left from #796 and not part of this commit: the Nft bulk import, which also has to extend levels for new states (#763); the export/import round trip with #755's transitions_as_arrays() and pickling, which do not exist on devel yet; a lifetime test for noodle segments sharing one automaton; and a Python benchmark kept in the repository. The figures above were not re-measured after rebasing. Part of #796.
Adda0
force-pushed
the
issue-782-delta-builder-pr
branch
from
October 15, 2026 18:04
12dcfff to
a3d23c9
Compare
Adda0
force-pushed
the
issue-796-python-bulk-pr
branch
from
October 15, 2026 18:05
c160bf5 to
4efaf7c
Compare
This was referenced Oct 16, 2026
Adda0
added a commit
that referenced
this pull request
Oct 16, 2026
…mata_str (#957) `copy.copy(nfa)` and `copy.deepcopy(nfa)` raised "no default __reduce__ due to non-trivial __cinit__"; only the custom `deepcopy()` method existed, and it does not copy the label. `Nfa.__copy__` copies the automaton with the C++ copy constructor and shares the label; `Nfa.__deepcopy__` also deep-copies the label and registers the copy in the memo, so an automaton referenced twice in a deep-copied container stays a single object. The alphabet is shared in both cases, as the C++ copy constructor does. `Nfa.to_mata_str(alphabet=None)` binds `Nfa::print_to_mata`, as `Nft.to_mata_str()` already does for transducers. Symbols are named from the given alphabet, else from the automaton's own alphabet, else printed as numbers. Part of #755. ### Tests `test_nfa.py`: `to_mata_str()` with numeric symbols, with an explicit alphabet and with the automaton's own alphabet; `copy.copy`/`copy.deepcopy` keep the states, the initial and final states and the delta, are independent of the original, share/copy the label, and a label that refers back to the automaton resolves to the copy. Dropping the memo registration fails that last check. No performance claim: these are missing features of the issue (items c). ### Overlap No overlap with #880 or #895; rebased onto #892 (merged) without conflicts. Independent of the other #755 PRs; all of them merge together without conflicts.
Adda0
added a commit
that referenced
this pull request
Oct 16, 2026
…rom the export
Exporting the transitions built one Python `Transition` object per
transition. `Nfa.transitions_as_arrays()` instead fills three parallel
int64 `array('q')` buffers (sources, symbols, targets) in one C++ pass
over the delta, in the order of `iterate()`. The pass holds the GIL: a
thread mutating the delta mid-pass would invalidate the iterators and
overrun the counted buffers.
`to_dataframe()` builds its columns from these buffers, wrapped with
`numpy.frombuffer` (numpy comes with pandas; pandas copies an
`array.array` element by element but an ndarray in one go). The columns
keep their int64 dtype.
`Nfa` could not be pickled ("no default __reduce__ due to non-trivial
__cinit__"), so it could not be passed to `multiprocessing` workers.
`Nfa.__reduce__` now pickles the number of states, the initial and the
final states, the columnar export and the alphabet; the rebuild imports
the transitions with `Delta.add_transitions` (#892). The label is the
pickle state, restored by `Nfa.__setstate__` after the automaton is
memoized, so a label referring back to the automaton round-trips.
Pickling an automaton whose alphabet cannot be pickled raises
`TypeError`, as before, instead of silently dropping the alphabet.
Part of #755.
Adda0
added a commit
that referenced
this pull request
Oct 16, 2026
… once (#958) `Delta.state_post()`, `Delta.get_transitions_from()` and `Nfa.get_transitions_from_state()` copied the whole `StatePost` and then its `to_vector()` before converting it to Python objects. They now read the post through a const pointer into the delta; the two `SymbolPost` getters share one helper. `Segmentation.get_segments()` and `get_untrimmed_segments()` copied the segment vector by value, then each segment, then assigned its initial and final states and its delta into a fresh `Nfa`, which also dropped the segment's alphabet. The getters are now declared to return the const reference the C++ API returns, and each segment is copied once into the wrapping `Nfa`, alphabet included. Part of #755. ### Measurements Release bindings, `taskset -c 13`, each value a median of 7 calls, 3 alternating runs, median shown. Posts: one state with 2000 symbol posts of 8 targets each. Segments: a segment automaton of 4 segments with 5000 states each. | | devel | this branch | |---|---|---| | `Delta.state_post()` | 0.94 ms | 0.68 ms | | `Nfa.get_transitions_from_state()` | 0.96 ms | 0.59 ms | | `Delta.get_transitions_from()` | 5.18 ms | 3.17 ms | | `Segmentation.get_segments()` | 7.60 ms | 1.33 ms | | `Segmentation.get_untrimmed_segments()` | 33.06 ms | 10.12 ms | Measured on devel `ba4a338a78`; the commits since (#948, #910, #890, #883, #892, #944) do not change these code paths. ### Tests `test_strings.py`: segments (trimmed and untrimmed) keep the alphabet of the segmented automaton and are independent copies; this fails on `devel` (`segment.alphabet` is `None`). `test_trans.py`: state posts and transitions from a state with several targets per symbol, from a state without transitions, and from a state past the delta. ### Overlap - #892 (merged) already reads posts in place in `get_trans_from_state_as_sequence()` and `iter_transitions_from()`; this PR does the same for the three getters #892 left copying. - No overlap with #880 or #895. Independent of the other #755 PRs; all of them merge together without conflicts.
Adda0
added a commit
that referenced
this pull request
Oct 16, 2026
…rom the export (#956) Exporting the transitions built one Python `Transition` object per transition. `Nfa.transitions_as_arrays()` instead fills three parallel int64 `array('q')` buffers (sources, symbols, targets) in one C++ pass over the delta, in the order of `iterate()`. The pass holds the GIL: a thread mutating the delta mid-pass would invalidate the iterators and overrun the counted buffers. `to_dataframe()` builds its columns from these buffers, wrapped with `numpy.frombuffer` (numpy comes with pandas; pandas copies an `array.array` element by element but an ndarray in one go). The columns keep their int64 dtype. `Nfa` could not be pickled ("no default __reduce__ due to non-trivial __cinit__"), so it could not be passed to `multiprocessing` workers. `Nfa.__reduce__` now pickles the number of states, the initial and the final states, the columnar export and the alphabet; the rebuild imports the transitions with `Delta.add_transitions` (#892). The label is the pickle state, restored by `Nfa.__setstate__` after the automaton is memoized, so a label referring back to the automaton round-trips. Pickling an automaton whose alphabet cannot be pickled raises `TypeError`, as before, instead of silently dropping the alphabet. Part of #755. ### Measurements Release bindings, 1 000 000 transitions (50 000 states, 64 symbols), `taskset -c 12`, each value a median of 5 calls, 3 alternating runs, median shown: | | devel | this branch | |---|---|---| | `to_dataframe()` | 1236 ms | 37 ms | | `transitions_as_arrays()` | — | 35.1 ms | | `list(iterate())`, per-transition objects, for comparison (code unchanged; the gap is run-to-run noise) | 286 ms | 163 ms | Column dtypes are `int64` before and after. Measured on devel `ba4a338a78`; the commits since do not change these code paths (#957 adds `copy` support and `to_mata_str` next to them). The fill now holds the GIL instead of releasing it, one release/acquire pair less per call, so the single-threaded figures stand. ### Tests `test_nfa.py`: the columnar export equals `iterate()` element by element and has typecode `q` (also on an automaton with no transitions); `to_dataframe()` keeps three int64 columns; a pickle round trip keeps the number of states (including an isolated state), initial and final states, the delta and the label, and the restored automaton is independent; a label that is the automaton itself round-trips to the restored automaton (`RecursionError` before the label moved to the pickle state); an unpicklable alphabet raises `TypeError`; an `Nfa` passes to a `spawn` multiprocessing worker. Replacing the pickled state count with 0 fails the round-trip and the multiprocessing test. ### Overlap - Builds on #892 (merged): the unpickling rebuild uses its `Delta.add_transitions`. - #880 changes `to_networkx_graph`, so this PR leaves that function on `iterate()`. - Rebased onto #957 (merged), which adds `__copy__`/`__deepcopy__` next to `__reduce__`; independent of the other #755 PRs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Building an automaton from Python went one transition at a time through
Nfa.add_transition or Delta.add: one Python call, one argument conversion and
one sorted insertion per transition.
Delta gains two bulk entry points over the C++ batched import
Delta::add(std::vector&&): add_transitions(sources, symbols,
targets) taking three equal-length sequences, and add_transitions_from(triples)
taking an iterable of (source, symbol, target). Both convert and range-check the
whole input into C++ vectors before touching the delta, so invalid input raises
and leaves the automaton exactly as it was, and both release the GIL around the
native sort and build. The semantics match repeated add(): duplicates collapse,
states up to the largest mentioned one are allocated, and an import into a
non-empty delta merges.
Measured in this workspace, 200000 random transitions over 50000 states:
per-transition add_transition 120.5 ms, add_transitions 59.5 ms, a 2.02x
speedup, both producing identical automata. Peak RSS grows by 66 MB for the
per-transition loop and by 72 MB for the bulk import, which additionally holds
the three input sequences and the converted vector at the same time, so the
bulk path trades a little peak memory for half the time.
The two "TODO: Refactor into a generator" are resolved: iter_transitions_from(state)
streams the post of one state by reference, get_trans_from_state_as_sequence()
is a list of it, and iterate() is documented as the streaming form of
get_trans_as_sequence(). Return types are unchanged.
The ownership rules are now written down in the .pxd and .pyx docstrings and
each has a test: Nfa and Nft own the C++ automaton through a shared_ptr; a Delta
view holds an aliasing shared_ptr and keeps its automaton alive; the iteration
generators keep their owner alive; Transition, SymbolPost and Run own a raw heap
object.
The shared_ptr TODO in nfa.pxd is resolved by measurement rather than deleted
blindly: creating and dropping a Delta view costs 128 ns and a cheap method call
through thisptr.get() costs 82 ns, so the atomic reference count is not visible
next to the Python call overhead itself. The TODO is replaced by that number.
Left from #796 and not part of this commit: the Nft bulk import, which also has
to extend levels for new states (#763); the export/import round trip with
#755's transitions_as_arrays() and pickling, which do not exist on devel yet;
a lifetime test for noodle segments sharing one automaton; and a Python
benchmark kept in the repository. The figures above were not re-measured
after rebasing.
Part of #796.
Merge order: this PR is stacked on #883, which adds the C++ batched import
Delta::add(std::vector&&) it wraps. Merge #883 first; this PR's
base is #883's branch until then.