-
Notifications
You must be signed in to change notification settings - Fork 54
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
parallelize USF and NUSF algorithms using Dask #1198
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1198 +/- ##
==========================================
+ Coverage 38.54% 38.74% +0.20%
==========================================
Files 163 165 +2
Lines 36880 37038 +158
Branches 6106 6128 +22
==========================================
+ Hits 14214 14351 +137
- Misses 21553 21564 +11
- Partials 1113 1123 +10 ☔ View full report in Codecov by Sentry. |
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.
Looks great to me! Tests pass, I've ran a few examples and I can see the speed up (especially for larger number of random starts), and the usf_benchmark tool also works great and helps to see the improvements
@kbuma I can confirm that using the flag --sdoe_use_dask turns on the use of Dask for SDoE parallelization. I guess I was confused because of the small change in the printing statements to the console. Thank you! |
pinned Dask to <2024.3 until dask/dask#10998 gets resolved |
@sotorrio1 & @henry-gatech could we get a review here for this? |
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.
@kbuma can you double-check that the way I resolved the Git conflicts makes sense?
I had a question about bokeh
and a few minor comments, but none of them is blocking, so feel free to ignore them.
@@ -89,8 +89,10 @@ | |||
# Required packages needed in the users env go here (non-versioned strongly preferred). | |||
# requirements.txt should stay empty (other than the "-e .") | |||
install_requires=[ | |||
"bokeh!=3.0.*,>=2.4.2", |
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.
Is bokeh
needed for something in particular? I've tried both pip show bokeh
and searching for imports in the FOQUS code, and I couldn't find anything.
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.
bokeh
is required to view the dask dashboard that provides live monitoring of Dask computations (https://docs.dask.org/en/latest/dashboard.html)
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.
OK thanks, the dashboard looks really cool! It looks like bokeh
doesn't come with very onerous requirements of its own, so I think we can keep that in for the moment, and then revisit if/when it causes problems down the line.
Co-authored-by: Ludovico Bianchi <lbianchi@lbl.gov>
Fixes/Addresses:
#1193
Summary/Motivation:
In order to improve the performance of the USF and NUSF algorithms for SDoE we parallelize them utilizing Dask.
We verify that the parallelized implementation provides the same result as the original algorithm with performance gain (USF 2.5x across platforms; NUSF 3.5x on Windows).
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: