-
Notifications
You must be signed in to change notification settings - Fork 2
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-40441: Avoid use of deprecated Pipeline.toExpandedPipeline() #35
Conversation
Pre-commit and GitHub actions seem to have gotten out of sync on Black versions here. Any opinions on which one I should make the winner? |
@@ -81,7 +74,8 @@ def make_injection_pipeline( | |||
reference_pipeline: Pipeline | str, | |||
injection_pipeline: Pipeline | str | None = None, | |||
exclude_subsets: bool = False, | |||
excluded_tasks: set[str] | str = { | |||
excluded_tasks: set[str] | |||
| str = { |
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.
This looks wrong. Which version of black is doing that?
pre-commit autoupdate
will fix things for you. The version of black in there is a year old.
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.
That was pre-commit, so that old version of black.
4a88384
to
c292fa6
Compare
c292fa6
to
3ac26e5
Compare
@@ -3,7 +3,7 @@ astropy >= 4.0 | |||
esutil >= 0.6.9 | |||
galsim >= 2.4.6 | |||
git+https://github.com/lsst/daf_butler@main#egg=lsst-daf-butler |
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.
Whilst you are in here, can you modernize the format of the file?
lsst-daf-butler @ git+https://github.com/lsst/daf_butler@main
lsst-pipe-base @ git+https://github.com/lsst/pipe_base@main
lsst-utils @ git+https://github.com/lsst/utils@main
lsst-resources @ git+https://github.com/lsst/resources@main
lsst-pex-config @ git+https://github.com/lsst/pex_config@main
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, I just realized I confused this message with the one in ctrl_mpexec (which I did) and didn't do it. @leeskelvin, could you do this next time you're in the area?
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.
Actually it looks like I'll be doing it myself on a hotfix branch now because I also failed to drop the branch dependencies. Really botched this merge.
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.
Consider copying over the middleware checker for exactly that situation.
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.
Fixed on #36, but I didn't include the middleware checker.
No description provided.