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-35675: Remove Gen2 #267

Merged
merged 9 commits into from Jul 26, 2022
Merged

DM-35675: Remove Gen2 #267

merged 9 commits into from Jul 26, 2022

Conversation

timj
Copy link
Member

@timj timj commented Jul 26, 2022

Checklist

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

No code uses it any more.
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #267 (75719fa) into main (e718c22) will increase coverage by 9.39%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
+ Coverage   72.17%   81.57%   +9.39%     
==========================================
  Files          60       57       -3     
  Lines        6779     5938     -841     
  Branches     1399     1221     -178     
==========================================
- Hits         4893     4844      -49     
+ Misses       1658      868     -790     
+ Partials      228      226       -2     
Impacted Files Coverage Δ
python/lsst/pipe/base/connections.py 72.88% <ø> (ø)
python/lsst/pipe/base/pipelineIR.py 86.26% <ø> (ø)
python/lsst/pipe/base/task.py 78.63% <ø> (ø)
python/lsst/pipe/base/argumentParser.py 76.92% <63.63%> (+74.04%) ⬆️
python/lsst/pipe/base/cmdLineTask.py 53.57% <100.00%> (+50.96%) ⬆️
python/lsst/pipe/base/__init__.py 84.61% <0.00%> (-15.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e718c22...75719fa. Read the comment docs.

timj added 2 commits July 26, 2022 09:46
They are now stubs to allow existing code to still import it
but no gen2 execution can happen.
There are other CmdLineTask docs that need to be examined.
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

I'm not sure if the version strings for the deprecations are correct. Otherwise, an impressive score of +53 -2847!



@deprecated(
reason="Gen2 DataIdContainer is no longer supported. This functionality has been disabled.",
version="v23.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

In all of these, should the version listed be v23.0 (current) or v24.0 (next), thus promising that this routine will be in v24 but once that is released can be deleted? Reading the docs is ... unclear ... but that's how I read it: https://developer.lsst.io/stack/deprecating-interfaces.html#python-deprecation

Copy link
Member Author

Choose a reason for hiding this comment

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

We already promised that Gen2 was deprecated and could be removed any time as part of the v23 release. My statement is that CmdLineTask has been deprecated since v23 but now actually tells you it has been deprecated since then. I don't really see any point in keeping this code around for another two release cycles where it doesn't actually do anything and will break if something uses it. The "version" here is the version the code was formally deprecated. I have not included in the statements when we will actually delete the stubs but I was assuming I could do it as soon as nothing in lsst_distrib uses CmdLineTask.

Copy link
Member Author

Choose a reason for hiding this comment

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

I talked it over with @ktlim and he thinks that saying "since v23" is fine here.

@timj timj merged commit 26bb7a4 into main Jul 26, 2022
@timj timj deleted the tickets/DM-35675 branch July 26, 2022 21:48
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.

None yet

2 participants