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-33027: add PipelineGraph and supporting classes #221

Closed
wants to merge 6 commits into from

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented Dec 20, 2021

Checklist

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

@TallJimbo
Copy link
Member Author

Note that these commits (in their original form) mostly predate the ButlerURI -> ResourcePath rename, even if this branch doesn't. I'm planning to change the naming on another branch that will build on this one.

python/lsst/pipe/base/configOverrides.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipelineIR.py Outdated Show resolved Hide resolved

Parameters
----------
expanded_tasks : `Mapping` [ `str`, `TaskIR` ], optional
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I like that instrument is not written, seems like it would make it easier to determine where something came from, and what it is intended for. And without it I dont think we can work toward the goal of having instrument help inform the registry what the right instrument is

Copy link
Member Author

Choose a reason for hiding this comment

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

This bothered me, too, but we absolutely can't afford to apply instrument overrides again (since other overrides may have been applied on top of those that should not be overridden), and that's what would happen if we left the instrument directive in. I think the alternative is to add a syntax for specifying an instrument while noting that its overrides have already been applied. Would you prefer that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would prefer some syntax for that, or even another top level key, like instrument_name that does nothing, but is persisted.

This comment got be thinking, because these are conceptually finalized pipelines I can see why we can not apply instruments a second time. However, these are not actually distinguishable from any other pipelines on load, there really is nothing that stops someone from doing additional overrides, such as passing something like --instrument on the command line, or even deriving a whole new pipeline from this as an import. I think this could be a useful feature (especially in testing for production when you need to quickly modify or something), but I also think it could be a foot gun if not taken into account. With this in mind, is there anything about this ticket you would like to re-evaluate?

python/lsst/pipe/base/pipelineIR.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipelineIR.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipeline.py Outdated Show resolved Hide resolved
python/lsst/pipe/base/pipelineIR.py Outdated Show resolved Hide resolved
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 2 times, most recently from bf1ce5a to 2273659 Compare January 28, 2022 16:46
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 5 times, most recently from edf0438 to 44ac030 Compare January 13, 2023 22:10
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 3 times, most recently from b1087ef to d976e5a Compare January 20, 2023 05:02
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 2 times, most recently from b77c85d to 7a893cb Compare March 2, 2023 04:16
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage: 88.27% and project coverage change: +1.52 🎉

Comparison is base (806fb6d) 82.82% compared to head (b9b2dc2) 84.35%.

❗ Current head b9b2dc2 differs from pull request most recent head 0f8420c. Consider uploading reports for the commit 0f8420c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
+ Coverage   82.82%   84.35%   +1.52%     
==========================================
  Files          66       77      +11     
  Lines        7242     8524    +1282     
  Branches     1410     1634     +224     
==========================================
+ Hits         5998     7190    +1192     
- Misses        999     1054      +55     
- Partials      245      280      +35     
Impacted Files Coverage Δ
python/lsst/pipe/base/tests/no_dimensions.py 0.00% <0.00%> (ø)
python/lsst/pipe/base/tests/pipelineStepTester.py 0.00% <0.00%> (ø)
tests/test_pipeTools.py 100.00% <ø> (ø)
python/lsst/pipe/base/connectionTypes.py 77.92% <50.00%> (-9.18%) ⬇️
python/lsst/pipe/base/pipeline.py 65.20% <67.34%> (-0.86%) ⬇️
python/lsst/pipe/base/pipeline_graph/_edges.py 69.71% <69.71%> (ø)
...ython/lsst/pipe/base/tests/mocks/_pipeline_task.py 51.61% <71.05%> (+17.68%) ⬆️
python/lsst/pipe/base/pipeTools.py 80.64% <72.72%> (-13.56%) ⬇️
...on/lsst/pipe/base/pipeline_graph/_dataset_types.py 83.09% <83.09%> (ø)
...hon/lsst/pipe/base/pipeline_graph/_task_subsets.py 89.18% <89.18%> (ø)
... and 9 more

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 6 times, most recently from 5b67ba0 to cda0f39 Compare March 7, 2023 20:16
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 3 times, most recently from 468b7e7 to 688f475 Compare March 14, 2023 14:19
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 5 times, most recently from 91f5c3a to e4b109f Compare March 27, 2023 15:02
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 2 times, most recently from e51265e to a16a939 Compare May 29, 2023 15:58
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 2 times, most recently from e8d3349 to bcf519d Compare June 10, 2023 19:40
@TallJimbo TallJimbo force-pushed the tickets/DM-33027 branch 5 times, most recently from f743700 to b1d8206 Compare June 23, 2023 16:55
Much of the code changed here is actually stuff I want to deprecate in
the future, once PipelineGraph has been integrated with more things.
In the meantime, this addresses much the duplication caused by adding
PipelineGraph.
@TallJimbo
Copy link
Member Author

I just resolved some very old comments, but I think I'm actually just going to close this PR and make a new one, because the new branch has almost no relation to the old one.

@TallJimbo TallJimbo closed this Jun 23, 2023
@TallJimbo TallJimbo changed the title DM-33027: add support for writing pipelines in expanded form DM-33027: add PipelineGraph and supporting classes Aug 4, 2023
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.

3 participants