-
Notifications
You must be signed in to change notification settings - Fork 91
Remove AutoMLSearch self-reference #2304
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
Codecov Report
@@ Coverage Diff @@
## main #2304 +/- ##
=======================================
- Coverage 99.9% 99.9% -0.0%
=======================================
Files 281 281
Lines 24606 24608 +2
=======================================
+ Hits 24578 24579 +1
- Misses 28 29 +1
Continue to review full report at Codecov.
|
| if self.search_iteration_plot: | ||
| self.search_iteration_plot.update() | ||
| # True when running in a jupyter notebook, else the plot is an instance of plotly.Figure | ||
| if isinstance(self.search_iteration_plot, SearchIterationPlot): |
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.
Need this check because the api of SearchIterationPlot.update and plotly.Figure update are now different.
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.
Got it, so this line was not covered to begin with right?
|
@freddyaboulton thanks for sharing these plots. If I am reading them correctly, is it right to say that fixing this leak results in a slight reduction in memory consumption? I see a drop in peak memory usage on the order of ~100MB. |
|
@dsherry Yes! |
|
|
||
| class SearchIterationPlot(): | ||
| def __init__(self, data, show_plot=True): | ||
| class SearchIterationPlot: |
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.
Before we kept a reference to the latest results by passing a reference to AutoMLSearch. In order to not have a self-reference but still be able to access the latest results, I'm adding results as an argument to init and update. The plot property of AutoMLSearch will always pass the latest results to the plot.
angela97lin
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, the memory plots look great 😁
| if self.search_iteration_plot: | ||
| self.search_iteration_plot.update() | ||
| # True when running in a jupyter notebook, else the plot is an instance of plotly.Figure | ||
| if isinstance(self.search_iteration_plot, SearchIterationPlot): |
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.
Got it, so this line was not covered to begin with right?
chukarsten
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.
This looks good. Good work, nice to see some solid memory usage results.
ea91450 to
eed5292
Compare
bchen1116
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.
Nice!
a92ba22 to
ade7b07
Compare
Pull Request Description
Fixes #2226
We see slightly lower memory usage at the peak (~500 mb) and the memory after the peak is lower and starts to decrease (line after peak has a negative slope as opposed to being constant).
Memory for all unit tests
Memory for automl tests
Plot still works for jupyter notebook
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.