Skip to content
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

Change fail_after&move_on_after to set deadline relative to entering. Add CancelScope.relative_deadline #3010

Merged
merged 32 commits into from
Sep 23, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Jun 6, 2024

Partially deals with #2512 by documenting current behavior.

This PR now works to fully resolve #2512 by adding a relative_deadline attribute to CancelScope, which is then used by fail_after and move_on_after. Upon entering the CancelScope the absolute deadline is then calculated as min(deadline, current_time() + relative_deadline).

This PR also reverts an old workaround in timeouts.py due to pycharm being unable to resolve @contextmanager for type checking.

TODO:

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 96.87500% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.58%. Comparing base (b834f73) to head (9961abc).
Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
src/trio/_core/_run.py 92.59% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3010      +/-   ##
==========================================
- Coverage   99.60%   99.58%   -0.02%     
==========================================
  Files         121      121              
  Lines       17882    17992     +110     
  Branches     3214     3245      +31     
==========================================
+ Hits        17811    17917     +106     
- Misses         50       52       +2     
- Partials       21       23       +2     
Files with missing lines Coverage Δ
src/trio/_core/_tests/test_run.py 100.00% <100.00%> (ø)
src/trio/_tests/test_timeouts.py 98.46% <100.00%> (+0.81%) ⬆️
src/trio/_timeouts.py 100.00% <100.00%> (ø)
src/trio/_core/_run.py 99.02% <92.59%> (-0.37%) ⬇️

@jakkdl
Copy link
Member Author

jakkdl commented Jun 6, 2024

(ignore the commit history, I created the branch in a messy way. Will squash the PR anyway)

@jakkdl jakkdl requested a review from Zac-HD June 7, 2024 10:59
Copy link
Member

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@Zac-HD
Copy link
Member

Zac-HD commented Jun 7, 2024

Per my comment on the issue,

I think it would make sense for trio.fail_after(x) to be relative to entering the context manager; trio.fail_at(trio.current_time() + x) seems a more natural way to express the relative-to-construction-time semantics. (and the same for trio.move_on_{at,after})

Either way, we should document the chosen semantics.

While I think that documenting this is a net improvement, it might also lead people to rely on the creation-time semantics for *_after in a way that makes changing that to enter-time semantics harder later. Would you be up for making that change and documenting it as a one-step PR?

@jakkdl
Copy link
Member Author

jakkdl commented Jun 10, 2024

Per my comment on the issue,

I think it would make sense for trio.fail_after(x) to be relative to entering the context manager; trio.fail_at(trio.current_time() + x) seems a more natural way to express the relative-to-construction-time semantics. (and the same for trio.move_on_{at,after})
Either way, we should document the chosen semantics.

While I think that documenting this is a net improvement, it might also lead people to rely on the creation-time semantics for *_after in a way that makes changing that to enter-time semantics harder later. Would you be up for making that change and documenting it as a one-step PR?

Sure. Started thinking about different ways of implementing it in the issue: #2512 (comment)

jakkdl and others added 2 commits June 20, 2024 22:07
… `fail_after` now use it, setting the deadline relative to entering the cs, instead of at initialization.
@jakkdl
Copy link
Member Author

jakkdl commented Jun 20, 2024

  1. See fail_after deadline is set on initialization not context entry #2512 (comment) for discussion on the behavior of relative_deadline after entering.
  2. I personally prefer None over math.inf quite a bit, but changing the behavior of CancelScope.deadline would be breaking for anybody accessing the value of the property in the inf/None case - so I should probably just change relative_deadline to also use inf instead of None.
  3. Need to write docstring for relative_deadline, but can probably mostly just take the stuff I wrote in the newsfragment.
  4. Need to update docstrings for move_on_after and fail_after to be explicit about deadline being relative to entering. (i.e. undo/invert what was originally changed in this PR).

The pycharm bug referred to in timeouts seem to be fixed https://youtrack.jetbrains.com/issue/PY-36444/PyCharm-doesnt-infer-types-when-using-contextlib.contextmanager-decorator and released over a year ago, so I think we can drop the workaround... except mypy now behaves slightly different in one test (?).

@jakkdl jakkdl changed the title [docs] Add explicit documentation for relative timeout semantics Add CancelScope.relative_deadline. Change fail_after&move_on_after to set deadline relative to entering Jun 20, 2024
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two major comments (somewhat touched on in the code comments):

  • I think this could be done with backwards compat
  • I really think that the interaction between deadline and relative_deadline is non-obvious. Prior to entering the context manager, setting one might override the other, depends on the other value. But when in the context manager, setting one will always override the other. I think we should disallow interacting with both through runtime errors; I can't think of any design that would fix this.

newsfragments/2512.breaking.rst Outdated Show resolved Hide resolved
newsfragments/2512.breaking.rst Outdated Show resolved Hide resolved
newsfragments/2512.breaking.rst Outdated Show resolved Hide resolved
Comment on lines 542 to 547
_deadline: float = attrs.field(default=inf, kw_only=True, alias="deadline")
# use float|None, or math.inf?
# or take it as an opportunity to also introduce deadline=None?
_relative_deadline: float | None = attrs.field(
default=None, kw_only=True, alias="relative_deadline"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think deadline + relative_deadline should be supported: we could deny that using a post-init thing from attrs. Maybe someone has a use case for it? I can't imagine anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can maybe imagine something, but it feels niche and almost certainly could be recreated with two nested scopes or custom functions.

But if we don't want simultaneous usage, and also don't want relative_deadline usage after entering, then I think we should go with a different approach than adding an attribute to CancelScope. In the linked issue I discussed other approaches, see e.g. "new class that transforms into CancelScope" in #2512 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Zac-HD because you have opinions.

I think what @Zac-HD proposed of disallowing setting relative deadlines inside the cancel scope would help somewhat, even though I still think that absolute deadlines vs relative deadlines is a weird concept to have before the context manager. (So namely being able to pass both here and being able to set both with their properties)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preferred semantics are:

  • CancelScope gets a new ._relative_deadline: float property, with no corresponding init argument
  • the *_after() scopes create a CancelScope with deadline=inf, and then set the attribute
  • on entry to the CancelScope, set the deadline to min(deadline, _relative_deadline + current_time()), and then set _relative_deadline = inf
    • this means that you get the same semantics for setting deadline as any other scope, but "relative deadlines" don't become a semantic thing for users to think about
  • it's an error to set the _relative_deadline if the scope has ever been entered

The existing deprecation logic is a bit ugly, but seems like the best option available overall.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this your preferred semantics? I'm mostly just seeing downsides to having the feature be this hidden, but still able to sneakily introduce confusing behavior if a user is sending around a CancelScope. If I have a function that takes a CancelScope as a parameter, I'd have to introspect the hidden attribute if I wanted to figure out what it would set the deadline to once entered. Maybe that's uncommon logic-wise, but surely people are writing logging statements like print(f"this cs will time out at {cs.deadline}") (before entering).

If we don't want to introduce relative deadlines as a primary feature, and avoid any stealthy gotchas, then I strongly think we should introduce a class that transforms into a CancelScope - to make sure that typing would catch any attempt at passing/saving the return value of fail/move_on_after to something that doesn't expect precisely that, and raise AttributeError if attempting to access deadline.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, the min variant also breaks trying to change the deadline, unless we unset relative_deadline upon setting deadline. So ultimately there's no reason to use min and we instead do some conditionals on whether values are None/inf.

cs = move_on_after(5)
# wait no I want to make the deadline later
cs.deadline = trio.current_time() + 7
with cs:
    print(cs.deadline - trio.current_time()) # prints 5

this is regardless of if the time is relative to construction or entering.

and of course

cs = move_on_after(5)
cs.deadline = cs.deadline + 2
with cs:
    print(cs.deadline)  # prints 5, not 7, which it would previously

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now agree that my proposal upthread is clearly bad 😅

src/trio/_core/_run.py Outdated Show resolved Hide resolved
src/trio/_core/_run.py Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented Jun 24, 2024

RTD is failing because towncrier appends link to the github issue at the end of the newsfragments, but since we're inside a code block that leads to invalid python syntax. Trying to append newlines to the rst file doesn't help, they get stripped. Not sure the code blocks are really needed, could probably just move that stuff into the docstrings, but lets deal with that when we've decided on the precise implementation.

src/trio/_core/_run.py Outdated Show resolved Hide resolved
# This was previously a simple @contextmanager-based cm, but in order to save
# the current time at initialization it needed to be class-based.
# Once that change of behaviour has gone through and the old functionality is removed
# it can be reverted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth putting a comment alongside related deprecation warning because someone might just find usages of the deprecation function and remove them rather than searching more in the file.

Comment on lines 3 to 9
.. code-block:: python

# this is equivalent to how trio.move_on_after(5) would function previously
cs = trio.move_on_at(trio.current_time() + 5)
...
with cs:
...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RTD is failing because towncrier appends link to the github issue at the end of the newsfragments, but since we're inside a code block that leads to invalid python syntax. Trying to append newlines to the rst file doesn't help, they get stripped.

We can deal with that by appending an empty RST comment \n..\n to the newsfragment; it might even be worth upstreaming that into towncrier (for cases where the last nonempty line is indented).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

turns out an issue was recently opened for it: twisted/towncrier#614

Comment on lines 542 to 547
_deadline: float = attrs.field(default=inf, kw_only=True, alias="deadline")
# use float|None, or math.inf?
# or take it as an opportunity to also introduce deadline=None?
_relative_deadline: float | None = attrs.field(
default=None, kw_only=True, alias="relative_deadline"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preferred semantics are:

  • CancelScope gets a new ._relative_deadline: float property, with no corresponding init argument
  • the *_after() scopes create a CancelScope with deadline=inf, and then set the attribute
  • on entry to the CancelScope, set the deadline to min(deadline, _relative_deadline + current_time()), and then set _relative_deadline = inf
    • this means that you get the same semantics for setting deadline as any other scope, but "relative deadlines" don't become a semantic thing for users to think about
  • it's an error to set the _relative_deadline if the scope has ever been entered

The existing deprecation logic is a bit ugly, but seems like the best option available overall.

@jakkdl
Copy link
Member Author

jakkdl commented Jun 26, 2024

I gave it a shot to instead implement this as a transforming class. The basic implementation is way cleaner, but the big downside is that it will break code that currently does

cs = trio.move_on_after(5)
with cs:
  cs.deadline = 7  # AttributeError: _RelativeCancelScope does not have an attribute `deadline`
# you need to instead write
with cs as cs2:
  cs2.deadline = 7

There's a couple ways to fix that, though it's gonna be somewhat ugly. Either properties/methods that mirror the properties/methods of CancelScope, or dunder magic with __getattr__/__setattr__. We can make them invisible to type checkers and raise warnings though, and error if the user tries to access deadline before the scope is entered (if timeout_from_enter=True).

@jakkdl
Copy link
Member Author

jakkdl commented Sep 12, 2024

I'm sorry this has taken me so long to get to; vacation & illness took their toll! I'm aiming for a full review over the weekend, but here are some initial thoughts while I have them:

❤️

  • I think that having two transitions (add deprecation, remove deprecation) is worse for users than just switching immediately to the new behavior. Anyone who wants the previous behavior can already switch fail_after(n) to fail_at(current_time() + n) and have that backwards-compatible.

The downside to this is it being a silent breaking change, and if a library starts relying on it they will have differing behavior depending on what trio version it runs against. So will definitely need a "major" version bump.

  • before the scope is entered, I'd make it an error to access the relative-deadline on a non-relative scope. Accessing the deadline on a relative scope before entry can raise a deprecation warning; assigning is deprecated and converts to a non-relative scope.
    This does resolve some of the instances of silent breakage, though not all. I'm not sure I think a DeprecationWarning is
  • better for introspection in IDEs, avoids the reassignment issues above, avoids duplicating some tricky code that might get out of sync later, etc.

This seems fine, and if end users really do want different typing between absolute and relative scopes they can get it with a protocol with is_relative: Literal[True] (which we could even add ourselves)

  • Assigning an inspect.Signature() object to the __signature__ attribute of a callable does the trick for Sphinx, and can be guarded by an if "sphinx" in sys.modules: to save the important latency the rest of the time. Neat trick to keep typecheckers and docs users happy 😉

For which issue was this?

I pushed a commit that changes the implementation, I have not updated all docs/docstrings/newsfragments pending further changes.

src/trio/_core/_run.py Show resolved Hide resolved
Comment on lines 753 to 756
if self._is_relative is True:
raise RuntimeError(
"unentered relative cancel scope does not have an absolute deadline",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this a bit smoother: emit a deprecation warning, and then return current_time() + relative_deadline

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does lead to behavior that's funky enough I might favor a RuntimeError, but DeprecationWarnings should be visible enough that it's proobably fine? See newly added tests

Comment on lines 761 to 764
if self._is_relative is True:
raise RuntimeError(
"unentered relative cancel scope does not have an absolute deadline",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too, I'd emit a deprecation warning but set the deadline as expected, and if-not-entered also set is_relative=False _relative_deadline=inf.

Copy link
Member Author

@jakkdl jakkdl Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this one I have no problems with raising a warning instead, fixed.

src/trio/_core/_run.py Outdated Show resolved Hide resolved
src/trio/_tests/test_timeouts.py Outdated Show resolved Hide resolved
@@ -540,11 +540,21 @@ class CancelScope:
_has_been_entered: bool = attrs.field(default=False, init=False)
_registered_deadline: float = attrs.field(default=inf, init=False)
_cancel_called: bool = attrs.field(default=False, init=False)
_is_relative: bool | None = attrs.field(default=False, init=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can actually avoid tracking this attribute at all, and instead define:

@property
def is_relative(self):
    if self.deadline < inf and not self._has_been_entered:
        assert self.relative_deadline == inf
        return True
    if self.relative_deadline < inf and not self._has_been_entered:
        return False
    return None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, that works.
Re < inf vs != inf: we made the timeout helper functions have special handling for nan, but not CancelScope... oops

@Zac-HD
Copy link
Member

Zac-HD commented Sep 12, 2024

edited my comment above to point to the possible docs issue. I wouldn't include the protocol, if anyone wants that they can define it downstream.

@A5rocks
Copy link
Contributor

A5rocks commented Sep 13, 2024

The downside to this is it being a silent breaking change, and if a library starts relying on it they will have differing behavior depending on what trio version it runs against. So will definitely need a "major" version bump.

Yeah, not sure this is great. Even across a large version bump. ... Is there any actual use case for constructing move_on_after before the async with? If not, we could just warn about the deprecation for that specifically (check whether CancelScope was constructed too long before __enter__, no way to opt out), and not have to un-deprecate anything! (after a version or two, we could remove the deprecation)

@Zac-HD
Copy link
Member

Zac-HD commented Sep 13, 2024

Annoyingly, I think there's no good reason to construct a move_on_after in advance yet: with the current semantics you can more clearly express the behavior with a move_on_at(current_time() + n). Unfortunately the whole reason we're doing this is that you might want to express something like "when you eventually send this request, fail if it takes more than five seconds".

I guess we could make a deprecation-only release and then un-deprecate it when the semantics change, but I think it's less disruptive overall to just switch to the new behavior 😕

@A5rocks
Copy link
Contributor

A5rocks commented Sep 13, 2024

I guess we could make a deprecation-only release and then un-deprecate it when the semantics change, but I think it's less disruptive overall to just switch to the new behavior 😕

That's actually a great way to do this. Though if users want relative timeouts and realize this upon seeing the deprecation message, then they have no real alternative (other than restructuring things) which does suck. (... which is better than just silently being incorrect? but not as good as silently becoming correct -- the deprecation is for people who don't want relative timeouts).

... Alternatively, we could mention passing relative_timeout to CancelScope in the deprecation message and just hope that having to turn trio.move_on_after(5) into trio.CancelScope(relative_timeout=5) isn't very common. (not great, though stylistic things like that can always be caught by flake8-async once deprecation period is over).

@Zac-HD
Copy link
Member

Zac-HD commented Sep 13, 2024

It just seems extremely strange for someone who doesn't want a relative timeout to use move_on_after(), so I still favor a one-step update where we just make that do the right thing. I don't think this will affect many users at all.

Good call on the linter though: we could have a lint rule against calling *_after() without immediately entering it, which only fires if the Trio version is older than whatever this patch ends up being. And then add a config option for trio_version (and anyio_version?), which defaults to whatever is installed.

@A5rocks
Copy link
Contributor

A5rocks commented Sep 13, 2024

Yeah ok I agree that it's fine to do this without deprecation. It's not that big of a behavior change (your code runs for just a bit longer) and deprecation strategies range from counter-productive to very weird (making people use the relative_timeout parameter). And if the linter could catch old usages that would be very nice.

@jakkdl
Copy link
Member Author

jakkdl commented Sep 16, 2024

Good call on the linter though: we could have a lint rule against calling *_after() without immediately entering it, which only fires if the Trio version is older than whatever this patch ends up being. And then add a config option for trio_version (and anyio_version?), which defaults to whatever is installed.

I think gating it on Trio version isn't great, because it's fairly easy to get mismatched versions between what your linter/ci setup is running (very easy if you're using pre-commit), and if you're supporting a range of trio versions you almost surely won't have different environments that lints against different versions of dependencies. So I'd go with a default-enabled check, that mentions that you should disable it if you only support Trio>0.x (and at some point we might default-disable that check)
Opened python-trio/flake8-async#290, we can have further linter-specific discussions in there.

@jakkdl
Copy link
Member Author

jakkdl commented Sep 16, 2024

* Assigning an `inspect.Signature()` object to the `__signature__` attribute of a callable does the trick for Sphinx, and can be guarded by an `if "sphinx" in sys.modules:` to save the important latency the rest of the time.  Neat trick to keep typecheckers _and_ docs users happy 😉
  (edit: to address [Add `_RelativeCancelScope`. Change `fail_after`&`move_on_after` to set deadline relative to entering #3010 (comment)](https://github.com/python-trio/trio/pull/3010#issuecomment-2331056189) )

Neat! But I now realized we in fact probably shouldn't do this because it does actually matter: #3010 (comment)

The only way (other than adding a wrapping _FailingCancelScope 🙃) I can see to fix it is adding a parameter to CancelScope that makes it raise TooSlowError when canceled. But that's perhaps for another issue

Copy link
Member Author

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed all comments from Zac. newsfragments/docs still need another pass

Comment on lines 181 to 184
with pytest.raises(
ValueError,
match="^(duration|deadline|timeout) must (not )*be (non-negative|NaN)$",
match="^(duration|deadline|timeout|relative deadline) must (not )*be (non-negative|NaN)$",
):
Copy link
Member Author

@jakkdl jakkdl Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a mess. For non-relative timeouts the term is always "deadline", but for relative timeouts there are multiple terms used in different parts of the code:

Using relative_deadline everywhere probably isn't the best (await trio.sleep(relative_deadline=5)), but having four terms for essentially the same thing is very ugly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd just update the error messages to refer to the argument name in all cases, using backticks to make it clear that this is an argument name if needed (e.g. maybe for "seconds"?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since both move_on_after and CancelScope want to validate the same input parameter but currently have a different name for it, that would mean one of these has to be true:

  • move_on_after(seconds=-1) gives "relative_deadline" can't be negative
  • we duplicate validation code between move_on_after and CancelScope.__attrs_post_init__
  • move_on_after catches and reraises the exception from CancelScope solely to replace the parameter name in the error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating validation logic seems least bad of these options 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another option we could do. Given how many names this parameter has, we could just add an additional parameter to CancelScope itself, with the name to use when reporting errors. Maybe that clutters the API too much though. But it does seem anyone else wrapping might also want to change the error reporting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds quite overcomplicated imo, esp since that would probably include both deadline and relative_deadline. If users are wrapping it and want pretty errors I don't think it's unreasonable to expect them to catch and reraise

@jakkdl jakkdl changed the title Add _RelativeCancelScope. Change fail_after&move_on_after to set deadline relative to entering Change fail_after&move_on_after to set deadline relative to entering. Add CancelScope.relative_deadline Sep 18, 2024
@jakkdl
Copy link
Member Author

jakkdl commented Sep 18, 2024

Once python-trio/flake8-async#292 is merged I'll update the newsfragment to refer to ASYNC122, otherwise all docs/docstrings/etc should be good now.

@jakkdl jakkdl requested a review from Zac-HD September 19, 2024 11:03
@Zac-HD Zac-HD merged commit f03663a into python-trio:main Sep 23, 2024
38 checks passed
@jakkdl jakkdl deleted the fail_after_documentation branch September 24, 2024 10:19
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.

fail_after deadline is set on initialization not context entry
5 participants