-
-
Notifications
You must be signed in to change notification settings - Fork 748
Transition table as a ClassVar #6331
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
Conversation
|
|
||
| return ws | ||
|
|
||
| def set_duration_estimate(self, ts: TaskState, ws: WorkerState) -> float: |
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 just a cut-paste out of the transitions section, as this is not a transition-specific method
ff1b15e to
3233dee
Compare
| ("ready", "released"): transition_generic_released, | ||
| ("released", "error"): transition_generic_error, | ||
| ("released", "fetch"): transition_released_fetch, | ||
| ("released", "missing"): transition_generic_missing, |
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.
Includes one-liner fix from #6327
3233dee to
b3ca1a4
Compare
hendrikmakait
left a 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.
LGTM
| # { | ||
| # (start, finish): | ||
| # transition_<start>_<finish>( | ||
| # self, ts: TaskState, *args, stimulus_id: str | ||
| # ) -> (recommendations, instructions) | ||
| # } |
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.
🙌
distributed/worker.py
Outdated
|
|
||
| start = ts.state | ||
| func = self._transitions_table.get((start, cast(str, finish))) | ||
| func = self.TRANSITIONS_TABLE.get((start, cast(TaskStateState, finish))) |
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.
nit: Should we generally keep the _ around to mark the transitions table as internal use only, i.e. use _TRANSITIONS_TABLE in both Scheduler and Worker?
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.
sure
distributed/worker.py
Outdated
| # self, ts: TaskState, *args, stimulus_id: str | ||
| # ) -> (recommendations, instructions) | ||
| # } | ||
| TRANSITIONS_TABLE: ClassVar[ |
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.
Meta question: Do we have any conventions on the ordering of class variable definitions, instance variable declarations, class/instance method definitions, etc?
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.
Typically class variables -> instance variables -> methods. In this case however the class variable MUST be after the transitions.
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, I was just wondering how strict we'd be here, also when it comes to ordering of public/private, class/instance methods. It looks like the rule there is "whatever seems to be a reasonable order/grouping".
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 don't group public and private separately. e.g. typically we put private helper methods next to their public parent methods.
__init__methodCC @hendrikmakait