Skip to content

graphlib.TopologicalSorter.prepare() should be idempotent #130914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lordmauve opened this issue Mar 6, 2025 · 5 comments · Fixed by #131317
Closed

graphlib.TopologicalSorter.prepare() should be idempotent #130914

lordmauve opened this issue Mar 6, 2025 · 5 comments · Fixed by #131317
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@lordmauve
Copy link
Contributor

lordmauve commented Mar 6, 2025

Feature or enhancement

Proposal:

Proposed Behaviour

TopologicalSorter.prepare() should be idempotent such that calling it repeatedly without draining anything is not an error:

>>> import graphlib
>>> ts = graphlib.TopologicalSorter()
>>> ts.prepare()
>>> ts.prepare()

If you have called .get_ready()/.done() then calling prepare() is probably a programming error and this would raise:

>>> import graphlib
>>> ts = graphlib.TopologicalSorter()
>>> ts.prepare()
>>> ts.get_ready()
()
>>> ts.prepare()
Traceback (most recent call last):
  ...
ValueError: cannot prepare() after mutating prepared sorter

Rationale

TopologicalSorter.prepare() raises an exception if you call it twice:

>>> import graphlib
>>> ts = graphlib.TopologicalSorter()
>>> ts.prepare()
>>> ts.prepare()
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    ts.prepare()
    ~~~~~~~~~~^^
  File "/home/mauve/.local/share/ext-python/python-3.13.0.82/lib/python3.13/graphlib.py", line 95, in prepare
    raise ValueError("cannot prepare() more than once")
ValueError: cannot prepare() more than once

This is rather unfortunate because if you return a TopologicalSorter that is prepared:

def get_sorter(targets) -> TopologicalSorter[str]:
    """Get a TopologicalSorter that pursues the given targets."""
    deps = filter_deps(load_deps(), targets)
    ts = TopologicalSorter(deps)
    ts.prepare()
    return ts

because then you cannot run .static_order() on it:

>>> get_sorter().static_order()
Traceback (most recent call last):
  ...
ValueError: cannot prepare() more than once

while if you don't prepare(), you then require the caller to do it, meaning the function that populates and returns a TopologicalSorter didn't leave it in a prepared state. It seems appropriate for such a function to call prepare() in order to leave the TopologicalSorter ready to iterate and also closed for the addition of new nodes/predecessors.

Therefore I think prepare() should be idempotent.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

This is related to #91301 which discusses removing TopologicalSorter.prepare() entirely. Doing so would bypass this issue.

Linked PRs

@lordmauve lordmauve added the type-feature A feature request or enhancement label Mar 6, 2025
@picnixz picnixz added the stdlib Python modules in the Lib dir label Mar 7, 2025
@vstinner
Copy link
Member

cc @pablogsal

@pablogsal
Copy link
Member

@tim-one what do you think Tim?

@tim-one
Copy link
Member

tim-one commented Mar 13, 2025

I like the simple rule .prepare() can be called at most once. I'd change the OP's get_sorter() to take an optional prepare=True argument, and leave it on the user to call that with False if they intend to call .static_order() (or in any other way want to alter the graph initially constructed).

That said, I think it would be easy to implement (if not to document) what the OP appears to want: .prepare(), when self._ready_nodes is not None, could just return if self._npassedout == 0.

Altermative: leave .prepare() alone and change static_order() instead, to start with:

if self._ready_nodes is None:
    self.prepare()
elif self._npassedout:
    raise ValueError("static_order() cannot be called after work has started")  

@tim-one
Copy link
Member

tim-one commented Mar 14, 2025

On second thought, .prepare() is being more dogmatic than necessary. Calling it again is harmless so long as ._npassedout is 0 (and in that case - ._ready_nodes is not None but .npassedout == 0 - it should just return at once). BTW, we cannot execute the body of .prepare() again in that case - we must just return. Else ._ready_nodes would become empty again, despite that the user never saw what was in it.

@lordmauve
Copy link
Contributor Author

One consequence of reusing _npassedout is that empty TopologicalSorters would let you call prepare after static_order():

>>> from graphlib import TopologicalSorter
>>> t = TopologicalSorter()
>>> list(t.static_order())
[]
>>> t.prepare()
>>> t.prepare()

While a non-empty one doesn't allow that:

>>> from graphlib import TopologicalSorter
>>> t = TopologicalSorter({'a': 'bc'})
>>> t.add(*'abc')
>>> list(t.static_order())
['b', 'c', 'a']
>>> t.prepare()
Traceback (most recent call last):
  ...
ValueError: cannot prepare() after starting sort

lordmauve added a commit to lordmauve/cpython that referenced this issue Mar 16, 2025
sebbASF added a commit to sebbASF/cpython that referenced this issue Mar 18, 2025
colesbury pushed a commit to colesbury/cpython that referenced this issue Mar 20, 2025
…python#131317)

Closes python#130914: Make graphlib.TopologicalSorter.prepare() idempotent

Relax the rules so that `.prepare()` can be called multiple times, provided that no work has been passed out by `.get_ready()` yet.
seehwan pushed a commit to seehwan/cpython that referenced this issue Apr 16, 2025
…python#131317)

Closes python#130914: Make graphlib.TopologicalSorter.prepare() idempotent

Relax the rules so that `.prepare()` can be called multiple times, provided that no work has been passed out by `.get_ready()` yet.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants