-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Conversation
Codecov ReportAttention: Patch coverage is
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
|
(ignore the commit history, I created the branch in a messy way. Will squash the PR anyway) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Per my comment on the issue,
While I think that documenting this is a net improvement, it might also lead people to rely on the creation-time semantics for |
Sure. Started thinking about different ways of implementing it in the issue: #2512 (comment) |
… `fail_after` now use it, setting the deadline relative to entering the cs, instead of at initialization.
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 (?). |
CancelScope.relative_deadline
. Change fail_after
&move_on_after
to set deadline relative to entering
There was a problem hiding this 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
andrelative_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.
src/trio/_core/_run.py
Outdated
_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" | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 aCancelScope
withdeadline=inf
, and then set the attribute - on entry to the
CancelScope
, set the deadline tomin(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
- this means that you get the same semantics for setting
- 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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😅
… a parameter to opt into new functionality and silence warning.
…use private names for attributes
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 |
src/trio/_timeouts.py
Outdated
# 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. |
There was a problem hiding this comment.
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.
newsfragments/2512.deprecated.rst
Outdated
.. 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: | ||
... |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
src/trio/_core/_run.py
Outdated
_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" | ||
) |
There was a problem hiding this comment.
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 aCancelScope
withdeadline=inf
, and then set the attribute - on entry to the
CancelScope
, set the deadline tomin(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
- this means that you get the same semantics for setting
- 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.
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 |
❤️
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.
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
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
Outdated
if self._is_relative is True: | ||
raise RuntimeError( | ||
"unentered relative cancel scope does not have an absolute deadline", | ||
) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 DeprecationWarning
s should be visible enough that it's proobably fine? See newly added tests
src/trio/_core/_run.py
Outdated
if self._is_relative is True: | ||
raise RuntimeError( | ||
"unentered relative cancel scope does not have an absolute deadline", | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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. |
Yeah, not sure this is great. Even across a large version bump. ... Is there any actual use case for constructing |
Annoyingly, I think there's no good reason to construct a 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 |
It just seems extremely strange for someone who doesn't want a relative timeout to use Good call on the linter though: we could have a lint rule against calling |
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. |
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) |
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 |
…dling to CancelScope. Move value validation from move_on_after/at to CancelScope
There was a problem hiding this 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
src/trio/_tests/test_timeouts.py
Outdated
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)$", | ||
): |
There was a problem hiding this comment.
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:
- parameter to
move_on_after
&fail_after
is called "seconds"- the error messages said "timeout must not..."
- the parameter in
CancelScope
isrelative_deadline
, and the error messages in it also use that term. - the parameter to
def sleep
is "seconds", but error message says "duration" asyncio.sleep
names the parameter "delay": https://docs.python.org/3/library/asyncio-task.html#asyncio.sleep- anyio.sleep also uses "delay"
time.sleep
names it "secs" 🤮 https://docs.python.org/3/library/time.html#time.sleep
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.
There was a problem hiding this comment.
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"?)
There was a problem hiding this comment.
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
andCancelScope.__attrs_post_init__
move_on_after
catches and reraises the exception fromCancelScope
solely to replace the parameter name in the error message.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
_RelativeCancelScope
. Change fail_after
&move_on_after
to set deadline relative to enteringfail_after
&move_on_after
to set deadline relative to entering. Add CancelScope.relative_deadline
Once python-trio/flake8-async#292 is merged I'll update the newsfragment to refer to |
Partially deals with #2512 by documenting current behavior.This PR now works to fully resolve #2512 by adding a
relative_deadline
attribute toCancelScope
, which is then used byfail_after
andmove_on_after
. Upon entering theCancelScope
the absolute deadline is then calculated asmin(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:
fail_after
andmove_on_after
_RelativeCancelScope
resolveNone
vsmath.inf
for no deadlineofrelative_deadline
after entering the cm. Seefail_after
deadline is set on initialization not context entry #2512 (comment)