Ensure that WatchedSubprocess inheritors have consistent method signatures #45537
Conversation
dstandish
requested review from
ephraimbuddy and
jedcunningham
as code owners
boring-cyborg
bot
added
area:Scheduler
| constructor_kwargs=dict( | ||
| selector=self.selector, | ||
| ), | ||
| target=_parse_file_entrypoint, |
There was a problem hiding this comment.
Both subclasses accept target -- do we need to pull this out here? It feels "leaky" to have a subclass but always have to pass the same value to it.
| def _on_child_started(self, *, requests_fd: int, **kwargs): | ||
| """Send startup message to the subprocess.""" | ||
| ti: TaskInstance = kwargs["ti"] | ||
| path: str | os.PathLike[str] = kwargs["path"] |
There was a problem hiding this comment.
Do we need to do this change? Other than mypy thinking that subclasses have to have the same signature, isn't this functionally identical?
There was a problem hiding this comment.
it's just for mypy, and whatever god enforces liskov substitution principle
| """The handle connected to stdin of the child process""" | ||
|
|
||
| client: Client | ||
| client: Client | None |
There was a problem hiding this comment.
What broke with leaving this as it was?
I wonder if instead of doing this change it might be better to have three classes -- the "base" WatchedSubproces, and then create a new class like:
class ActivitySubprocess(WatchedSubprocess):
id: UUID
client: Client
def _on_child_started(self, ti: TaskInstance, path: str | os.PathLike[str], requests_fd: int):
...There was a problem hiding this comment.
no client is provided by dag processor it was explicitly given none; so clearly the base class is ok with that. it's only for task execution scenario that it is used and depended on. that's why the type checking asserts
There was a problem hiding this comment.
Yeah but what broke by leaving it as it was?
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
Yeah, this was a bit messy I agree.
I think by having this as two subclasses (for a total of three classes) also means we can keep the public signature as it is taking ActivitySubprocess.start(dag_path, ti, client=client, logger=logger), and that could then pass the right things on to child_started_kwargs in the super call etc?
The dag processor does not pass a TI -- it only passes (optionally) a callback list. |
…tures The `start` and `_on_child_started` methods have inconsistent signatures for the dag processor subclass. This PR resolves that. Why bother? Well, I need to change the path handling a little bit to pass rel path and bundle info separately. And we should get rid of the dags folder concept. And when I tried to make these changes in the execution context, it broke dag parsing, because of this signature issue.
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
|
By having a separate subclass for execution we get rid of the need to have |
dstandish
force-pushed
the
unify-signatures-for-watched-subprocess-inheritors
branch
from
b05006d to
e7ea4bc
Compare
3 participants
The
startand_on_child_startedmethods have inconsistent signatures for the dag processor subclass.This PR resolves that.
Why bother? Well, I need to change the path handling a little bit to pass rel path and bundle info separately. And we should get rid of the dags folder concept. And when I tried to make these changes in the execution context, it broke dag parsing, because of this signature issue.