Closed talumbau closed 4 months ago
@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.
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
).
@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.
@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.
@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
.
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.
@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
).
@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.
@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.
global
and then pass to the client in the inner loop. This significantly reduces the run time of SS.