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.

Sign up for GitHub

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

Jump to bottom

DM-39377: Deprecate PipelineTaskConfig.saveMetadata field #341

Merged
merged 4 commits into from Jun 6, 2023
Merged

DM-39377: Deprecate PipelineTaskConfig.saveMetadata field #341

merged 4 commits into from Jun 6, 2023

Conversation

andy-slac
Copy link
Contributor

Also removes unused ResourceConfig class.

Checklist

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

@codecov
Copy link

codecov bot commented Jun 5, 2023 •
edited

Codecov Report

Patch coverage: 90.90% and project coverage change: -0.07 ⚠️

Comparison is base (ccb5b7f) 82.58% compared to head (e01d346) 82.52%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
- Coverage   82.58%   82.52%   -0.07%     
==========================================
  Files          62       61       -1     
  Lines        6925     6865      -60     
  Branches     1368     1363       -5     
==========================================
- Hits         5719     5665      -54     
+ Misses        929      926       -3     
+ Partials      277      274       -3
Impacted Files Coverage Δ
python/lsst/pipe/base/pipelineTask.py 69.56% <0.00%> (-2.44%) ⬇️
python/lsst/pipe/base/config.py 66.25% <100.00%> (-1.22%) ⬇️
python/lsst/pipe/base/pipeline.py 65.05% <100.00%> (+0.44%) ⬆️

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

timj
timj reviewed Jun 5, 2023
View reviewed changes
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Don't we also have to stop using the config in pipeline.py?

python/lsst/pipe/base/config.py Outdated
@@ -200,6 +200,7 @@ class to allow configuration of the connections class. This dynamically
default=True,
optional=False,
doc="Flag to enable/disable metadata saving for a task, enabled by default.",
deprecated="This field is deprecated and will be removed after v25.",
Copy link
Member

@timj timj Jun 5, 2023 •
edited

Choose a reason for hiding this comment

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

I think it has to be v26 because v25 is already being worked on from earlier in the year.

@andy-slac
Copy link
Contributor Author

Don't we also have to stop using the config in pipeline.py?

Hmm, I was thinking that we'll be doing some cleanup when we actually drop it completely. But I can certainly update things now to assume that saveMetadata is always true (ctrl_mpexec is the other place with that logic).

@timj
Copy link
Member

timj commented Jun 5, 2023

I think if we deprecate a config we are meant to stop using it.

@andy-slac
Deprecate PipelineTaskConfig.saveMetadata field (DM-39377)
6fe5aea 
Code is updated to always store task metadata.
@timj
Copy link
Member

timj commented Jun 6, 2023

I guess the change note should mention explicitly that the saveMetadata config is now ignored.

@andy-slac andy-slac merged commit e026563 into main Jun 6, 2023
9 of 10 checks passed
@andy-slac andy-slac deleted the tickets/DM-39377 branch June 6, 2023 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timj timj timj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@andy-slac @timj