-
Notifications
You must be signed in to change notification settings - Fork 21
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
Pipeline runner improvements #258
Conversation
41508a1
to
42e8cfb
Compare
48f50cd
to
bbb48dc
Compare
2a822b3
to
c6bdf71
Compare
c6bdf71
to
2f7ef4e
Compare
I think this is ready for a review and some thoughts on the priority implementation. I've been testing the daily pipeline config with good results, but I'm still trying to think of edge cases where this might fail/behave worse than the old implementation. Input is welcome. The memory tools seem to be working as expected. |
2f7ef4e
to
c33948f
Compare
81b7f74
to
361800f
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.
I don't see any real show-stoppers here. I think the new priority system is well designed: it's both simple (to explain/implement) but flexible enough to provide users the necessary tools to adjust things when necessary.
Some extra documentation on how it works may help.
You're right that there may be some edge cases that come up in production, but I don't see anything so obvious just looking at the PR.
The legacy ordering at least means the previous behaviour can be returned to until things are fixed, if everything goes pear-shaped. And I don't think we're locking ourselves into anything that's going to get us into trouble and we couldn't fix if needed.
I have left some comments which I think are mostly minor in scale.
361800f
to
6c2a65e
Compare
c55d7f5
to
f1afa3d
Compare
4a738f9
to
6e7f028
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.
This looks reasonable. It's difficult, with static analysis, to anticipate what sort of corner cases (if any) we might get ourselves into with this change, but I think this is implemented well enough that's it's a good place to start.
I found a couple of typos:
6128269
to
41e4a13
Compare
41e4a13
to
38a9a2c
Compare
This is a pretty involved PR, so it may take some time to review.
Features
This PR addresses most of the issues in #181 and #152. In particular:
available
, meaning that these will only run when nothing else is able to do anythingnext
ifsetup
did not run successfullylegacy
option which will mimic the current pipeline behaviourlimit_outputs
: restrict the number of times a task can produce output in thenext
state. Once this limit is reached, the task will immediately advance its state.base_priority
: priority modifier which is added to the calculated priority. Can be negative.Two other features are added which are helpful for memory analysis:
PSUtilProfiler
which checks memory usage every 0.5 seconds in an attempt to log peak memory used by a taskRefactoring
pipeline.Manager
class related to initial task loading and pipeline creation. This does not change any behaviour, but slightly improves the distribution of responsibility between methodsRemaining:
Note: this re-implements the multi-output functionality of #263, but that has some extra tests