-
Notifications
You must be signed in to change notification settings - Fork 120
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
vine: daskvine priority scheduling #3923
vine: daskvine priority scheduling #3923
Conversation
return dependencies | ||
|
||
def set_depth(self, key): |
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'm not sure about this change. Didn't the old way of computing depth worked?
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.
@btovar No, the old way didn't give me the right depth.
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.
In which way it didn't work? Do you have an example? Is it a different definition of graph depth?
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 tested it about 2 months ago, my impression is that the depth of every task in the graph was always 0 or 1 or something fixed, and then I just wrote a new function to calculate the depth.
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.
It did work for me when I was testing checkpoints. Please check again because it may indicate that something else in the code broke. Also, it is a much easier code to follow, which may help with dealing with future bugs.
taskvine/src/bindings/python3/ndcctools/taskvine/dask_executor.py
Outdated
Show resolved
Hide resolved
I reverted |
Sounds good, thanks! |
Proposed Changes
Give an overall description of the changes, along with the context and motivation.
Mention relevant issues and pull requests as needed.
Merge Checklist
The following items must be completed before PRs can be merge.
Check these off to verify you have completed all steps.
make test
Run local tests prior to pushing.make format
Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lint
Run lint on source code prior to pushing.