8000 Scatter Parameters to Workers once in SS by talumbau · Pull Request #911 · PSLmodels/OG-Core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Scatter Parameters to Workers once in SS #911

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 15, 2024
Merged

Scatter Parameters to Workers once in SS #911

merged 2 commits into from
Apr 15, 2024

Conversation

talumbau
Copy link
Member
  • Instead of scattering the Parameters object prior to each call to the dask client, scatter it once and put the resulting future in module level scope. Retrieve it using global and then pass to the client in the inner loop. This significantly reduces the run time of SS.

@talumbau talumbau requested review from rickecon and jdebacker March 16, 2024 21:19
@jdebacker
Copy link
Member

@talumbau PR #916 makes a change that gets the test_SS.py unit test passing. But not sure it's exactly what we want here.

@rickecon
Copy link
Member

@talumbau. PR #922 fixes the testing issues you were having, although this memory management issue in this PR will solve a bunch of broader testing issues we are having. See comments this and this in PR #922 thread.

talumbau added 2 commits April 13, 2024 17:20
@talumbau
Scatter Parameters to Workers once in SS
803189b 
 - Instead of scattering the Parameters object prior to each call to the
   dask client, scatter it once and put the resulting future in module
   level scope. Retrieve it using `global` and then pass to the client
   in the inner loop. This significantly reduces the run time of SS.
8000
@talumbau
Check if scatter_p is None
1774799 
if scatter_p is None from the global namespace, go ahead and
use the client to get the scattered version.
@codecov-commenter
Copy link
codecov-commenter commented Apr 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 73.40%. Comparing base (95987b7) to head (1774799).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
- Coverage   73.44%   73.40%   -0.05%     
==========================================
  Files          19       19              
  Lines        4610     4614       +4     
==========================================
+ Hits         3386     3387       +1     
- Misses       1224     1227       +3
Flag Coverage Δ
unittests 73.40% <57.14%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ogcore/SS.py 71.98% <57.14%> (-0.44%) ⬇️

@talumbau
Copy link
Member Author

@rickecon thanks so much for fixing the codecov situation! I'm looking at all greens here. If this looks good to you, let's merge!

also, agree on the idea of dropping 3.9 test coverage. Seems like a good way to reduce resource usage.

@rickecon
Copy link
Member

@talumbau and @jdebacker. I am comparing this PR to @jdebacker's PR #916. The only difference is that Jason's PR includes an else statement in the first `# Retrieve the "scattered" Parameters object" line. Both PRs are passing all the tests.

I recommend that, if it is correctly to include the else statement (which I think it is), TJ should just make that addition to this PR and we'll merge this and close Jason's.

Also, note that this PR is running both push and pull tests because TJ made a branch on this OG-Core repository. TJ, all the CI tests will run twice as fast (and Microsoft will hate us less) if you follow the workflow of forking this repository and submitting your PRs from branches of your fork.

I am adding an issue proposing to remove the Python 3.9 tests.

@jdebacker
Copy link
Member

@rickecon, @talumbau does include the else (in line) and offers an improvement over my changes in that it will use the global variable if it is not None.

@talumbau
Copy link
Member Author

Also, note that this PR is running both push and pull tests because TJ made a branch on this OG-Core repository. TJ, all the CI tests will run twice as fast (and Microsoft will hate us less) if you follow the workflow of forking this repository and submitting your PRs from branches of your fork.

Ah, sorry about that. I actually prefer the fork style anyway. Some other recent development work got me in this "push branch to origin" pattern, which I think is suboptimal. Will fork and PR from that from now on.

I am adding an issue proposing to remove the Python 3.9 tests.
Great idea.

@talumbau
Copy link
Member Author

@rickecon, @talumbau does include the else (in line) and offers an improvement over my changes in that it will use the global variable if it is not None.

Correct. This is the key feature. In the inner_loop the idea is to pull in scattered_p from the "global" (i.e. module-level) scope. In the "happy path" this is already set up as the correct future so we can just pass it to the dask call. So "do nothing" is what we hope to do in the inner loop. But, if it's None then we actually do the scatter. The good news is that, even if we do execute that scatter insider inner_loop it's a one time penalty, because the next time the Worker calls inner_loop the future is actually in the global namespace (and not None).

@rickecon
Copy link
Member

@jdebacker and @talumbau. OK. Then are we ready to merge this PR and close Jason's PR #916? It looks good to me. And I am excited to move on to the similar TPI solution.

@talumbau
Copy link
Member Author

@jdebacker and @talumbau. OK. Then are we ready to merge this PR and close Jason's PR #916? It looks good to me. And I am excited to move on to the similar TPI solution.

Yep. I will do the merge for this PR. I think we can close #916. Will have a similar one up for TPI soon (as a fork PR) and then will ask you both to do a before/after comparison just as a sanity check on what I'm getting on my workstation.

@talumbau talumbau merged commit 66d6223 into master Apr 15, 2024
@rickecon rickecon deleted the scatter_once branch April 15, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickecon rickecon rickecon approved these changes

@jdebacker jdebacker Awaiting requested review from jdebacker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@talumbau @jdebacker @rickecon @codecov-commenter
0