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
DM-42751: Add num_proc parameter to SeparablePipelineExecutor.run_pipeline() #283
Conversation
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.
I'm a bit concerned about the proliferation of executor-specific arguments in the run_pipeline
parameter list, but I can't deny the precedent. I've requested some documentation changes to keep the focus on SPE's intended usage.
@@ -241,6 +239,7 @@ def run_pipeline( | |||
graph: lsst.pipe.base.QuantumGraph, | |||
fail_fast: bool = False, | |||
graph_executor: QuantumGraphExecutor | None = None, | |||
num_proc: int = 1, |
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.
While I'm fine with changing the default to 1, I'm less fine about encoding that default in a parameter. The point of SeparablePipelineExecutor
is to let the user plug in customized components as needed. I'm now wondering how fail_fast
got in there; it doesn't seem to be something requested on #230.
Edit: making this very change was discussed on the old PR, but no mention of why I thought fail_fast
was an exception.
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.
Oops, looked only at the first commit.
The default of 80% of available cores without any direct means to adjust it with `run_pipeline` was causing issues in notebooks where we spawn new subprocesses rather than forking. The default has now changed to 1 core with no spawning.
10836d1
to
5529847
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
==========================================
- Coverage 87.41% 87.40% -0.01%
==========================================
Files 49 49
Lines 4433 4431 -2
Branches 764 764
==========================================
- Hits 3875 3873 -2
Misses 406 406
Partials 152 152 ☔ View full report in Codecov by Sentry. |
Checklist
doc/changes