- Notifications
You must be signed in to change notification settings - Fork6.6k
grid_search resolution code optimization#45267
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
Uh oh!
There was an error while loading.Please reload this page.
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>
This pull request has been automatically marked as stale because it has not had You can always ask for help on ourdiscussion forum orRay's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
This pull request has been automatically closed because there has been no more activity in the 14 days Please feel free to reopen or open a new pull request if you'd still like this to be addressed. Again, you can always ask for help on ourdiscussion forum orRay's public slack channel. Thanks again for your contribution! |
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.