- Notifications
You must be signed in to change notification settings - Fork6.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
grid_search resolution code optimization#45267
base:master
Are you sure you want to change the base?
Conversation
Signed-off-by: Robert Dyro <rdyro@stanford.edu>
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.
Thanks for the contribution! Code looks good, leaving one comment for understandability.
| # a skeleton is easier to copy for every iteration | ||
| unresolved_spec_skeleton = copy.deepcopy(unresolved_spec) | ||
| for path, _ in grid_vars: | ||
| assign_value(unresolved_spec_skeleton, path, None) | ||
| while value_indices[-1] < len(grid_vars[-1][1]): | ||
| spec = copy.deepcopy(unresolved_spec) | ||
| spec = copy.deepcopy(unresolved_spec_skeleton) |
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.
Could you clarify the comments here to describe what's happening in each step? Specifically I think it would be good to explain what the "skeleton" is (i.e. the spec with thegrid_vars set to none), and then within thewhile loop comment that we are populating the values for a single variant.
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.
That's a great suggestion; I'll add the comments ASAP.
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.
Let me know if that's what you had in mind or if you have any feedback.
…ere values are going to be filledSigned-off-by: Robert Dyro <rdyro@stanford.edu>
A small Python code optimization to significantly speed up
grid_searchresolution.Instead of deep-copying the whole unresolved spec for every resolved spec, we can create a skeleton spec, filled with None in place of grid variables, and deep-copy that every time. The fix involves a handful of line changes in one location.
I checked that the slow implementation is still present in the latest release.
Why are these changes needed?
The current grid resolution takes upwards of 1 minute on a Ryzen 7 7600X for grid_search generation for grids larger than 10k elements. Ray does not start the trials before generating the entire grid and the behavior, resulting from slow grid resolution, appears like ray has hung (as the trials are not starting) - this is confusing.
Related issue number
N/A
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.