Scatter Parameters to Workers once in SS

[talumbau]

• 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.

[jdebacker]

@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]

@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.

[codecov-commenter]

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

@@            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%)	:arrow_down:

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%)	:arrow_down:

[talumbau]

@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]

@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]

@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]

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]

@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]

@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]

@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.