Skip to content
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-44647: Move pipeline-dot build from cmdLineFwk to builder #292

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

leeskelvin
Copy link
Contributor

@leeskelvin leeskelvin commented Jun 6, 2024

This commit moves the pipeline-dot build logic from cmdLineFwk into cli/script/build.py.
As part of this commit, a switch to using the same back-end display args parser as --show pipeline-graph is also made.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

This commit moves the pipeline-dot build logic from cmdLineFwk into
cli/script/build.py.
As part of this commit, a switch to using the same back-end display args
parser as --show pipeline-graph is also made.
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.24%. Comparing base (f1e75d8) to head (28d8a12).

Files Patch % Lines
python/lsst/ctrl/mpexec/cli/script/build.py 25.00% 2 Missing and 1 partial ⚠️
python/lsst/ctrl/mpexec/cli/cmd/commands.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
- Coverage   87.25%   87.24%   -0.02%     
==========================================
  Files          50       50              
  Lines        4576     4578       +2     
  Branches      787      788       +1     
==========================================
+ Hits         3993     3994       +1     
- Misses        421      422       +1     
  Partials      162      162              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -582,9 +582,6 @@ def makePipeline(self, args: SimpleNamespace) -> Pipeline:
if args.save_pipeline:
pipeline.write_to_uri(args.save_pipeline)

if args.pipeline_dot:
pipeline2dot(pipeline, args.pipeline_dot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am now thinking how this PR relates to #159 given that this is the only usage of pipeline2dot and that PR was removing it completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fully supersedes #159. I'm also planning to RFC dropping support for QuantumGraph GraphViz outputs, since my understanding is that those are always too big for dot to render in practice anyway, and then we can get rid of dotTools.py entirely.

@leeskelvin leeskelvin force-pushed the tickets/DM-44647 branch 3 times, most recently from 66dac1b to 09ad34c Compare June 23, 2024 18:44
@leeskelvin leeskelvin requested a review from arunkannawadi June 23, 2024 18:46
@leeskelvin
Copy link
Contributor Author

I've removed the DO NOT MERGE commit from this PR. Note that the mypy check now fails. After this ticket has been approved, I'll merge the pipe_base branch to main first, then re-run the checks on this PR to ensure that all checks are successful prior to merge.

requirements.txt Outdated
lsst-pex-config @ git+https://github.com/lsst/pex_config@main
sqlalchemy
numpy >= 1.17, <2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this can be removed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Commit dropped.

@leeskelvin leeskelvin merged commit 2883a1f into main Jun 26, 2024
13 of 15 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-44647 branch June 26, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants