-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Improve startup times in Complement test runs against workers, particularly in CPU-constrained environments. #13127
Conversation
Signed-off-by: Olivier Wilkinson (reivilibre) <[email protected]>
966a376
to
757d66c
Compare
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.
looks pretty good to me! a couple of nits.
if attr_name == "___install": | ||
return self.___install |
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 this is redundant? __getattr__
is only called when a normal attribute lookup fails.
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 thought the same, but I get
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<stdin>", line 30, in __getattr__
AttributeError: 'NoneType' object has no attribute '___install'
if I don't have that there. :/
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.
'NoneType' object has no attribute '___install'
.... that implies you're calling ___install
on 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.
Oh, interesting, this isn't a problem if I only have one underscore as a prefix for the name. The docs don't seem to mention why this is, but oh well..
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.
'NoneType' object has no attribute '___install'
.... that implies you're calling
___install
onNone
?
Yes, because that's how __getattr__
is implemented. The underlying, not-yet-installed reactor (None) doesn't have an ___install
method.
According to what you + docs are saying, it shouldn't call __getattr__
because the class itself has a ___install
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.
if you want to try at home:
from typing import Any
class A:
def __init__(self) -> None:
self.___reactor_target: Any = None
def ___install(self, new_reactor: Any) -> None:
self.___reactor_target = new_reactor
def __getattr__(self, attr_name: str) -> Any:
return getattr(self.___reactor_target, attr_name)
class B:
def __init__(self) -> None:
self.___reactor_target: Any = None
def _install(self, new_reactor: Any) -> None:
self.___reactor_target = new_reactor
def __getattr__(self, attr_name: str) -> Any:
return getattr(self.___reactor_target, attr_name)
then:
a = A()
a.___install(42) # <--- fails
print(a.conjugate())
compare to
b = B()
b._install(42)
print(b.conjugate())
4340ac4
to
d677e52
Compare
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.
lgtm otherwise
Co-authored-by: Richard van der Hoff <[email protected]>
0537e13
to
3bd858f
Compare
Fixes #13051.
Should be more or less a step-by-step review, but it may be clear enough from the overall diff also, I don't know.