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-36649: Move ConfigurableActions from pipe_tasks #93

Merged
merged 7 commits into from Mar 23, 2023
Merged

Conversation

natelust
Copy link
Contributor

@natelust natelust commented Feb 8, 2023

Checklist

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

@timj timj changed the title DM-36649 DM-36649: Move ConfigurableActions from pipe_tasks Feb 8, 2023
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Patch coverage: 93.61% and project coverage change: +0.88 🎉

Comparison is base (79ca90b) 84.22% compared to head (5e486b4) 85.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #93      +/-   ##
==========================================
+ Coverage   84.22%   85.10%   +0.88%     
==========================================
  Files          40       46       +6     
  Lines        3176     3599     +423     
==========================================
+ Hits         2675     3063     +388     
- Misses        501      536      +35     
Impacted Files Coverage Δ
...ig/configurableActions/_configurableActionField.py 86.04% <86.04%> (ø)
...figurableActions/_configurableActionStructField.py 91.16% <91.16%> (ø)
...ython/lsst/pex/config/configurableActions/tests.py 92.10% <92.10%> (ø)
.../config/configurableActions/_configurableAction.py 92.85% <92.85%> (ø)
tests/test_configurableActions.py 99.30% <99.30%> (ø)
...on/lsst/pex/config/configurableActions/__init__.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@timj
Copy link
Member

timj commented Feb 8, 2023

Would you be up for doing the transfer by exporting the full history of the files from pipe_tasks, editing the file locations in the .patch files, and then using git am to add them back? That way we don't lose anything with the transfer.

@natelust
Copy link
Contributor Author

natelust commented Feb 8, 2023

Isn't the point of our code migration policy so that people can trace back where it came from based on commit messages and no-ff transfer branch merge?

But really in this case (in my opinion) there is not really enough history to bother with this.

@timj
Copy link
Member

timj commented Feb 8, 2023

I always copy the history over -- maybe everyone hates it when I do that (but moving the code out of pipe_drivers to pipe_tasks you will see the full history is retained -- no-one wants to have to dig pipe_drivers out of storage to do a git blame especially when the code has evolved later). If there is no history in this file that git blame would be useful for then okay.

@timj
Copy link
Member

timj commented Feb 8, 2023

@ktlim do you object to transferring history or is the policy the way it is because doing the git format-patch, manual editing, git am is quite tricky in general and not something we should require.

@TallJimbo
Copy link
Member

The policy in the dev guide is pretty thoroughly weighted towards "git blame is all important and making it hard to move code is fine"; I'm responsible for it, but these days I always either forget to follow it or regret having cooked it up (though it's been a while since I last did a transfer big enough that I think it'd apply).

@natelust
Copy link
Contributor Author

natelust commented Feb 8, 2023

here is the dev guide page that I followed, which I already find a needlessly complicated, though I do understand what the motivation is. It specifically mentions it makes no attempt to migrate the git history because it is retained in the original repository. Especially in this case, there is no meaningful history to worry about.

@natelust natelust force-pushed the tickets/DM-36649 branch 4 times, most recently from 4ab3647 to 7772f94 Compare February 8, 2023 17:45
@natelust
Copy link
Contributor Author

natelust commented Feb 8, 2023

Im not quite sure how to get github to stop trying python 3.8 and 3.9...

@timj
Copy link
Member

timj commented Feb 8, 2023

It's not trying to build them any more but the check is still listed as required. We need to change the branch protections to stop thinking that python 3.8 is needed.

@natelust
Copy link
Contributor Author

natelust commented Feb 8, 2023

Is that on github itself? I am not sure I have rights to do that

@timj
Copy link
Member

timj commented Feb 8, 2023

I fixed the checks. Don't forget to update pyproject.toml with the new version information (may as well add 3.11 to that list as well)

@gpdf
Copy link

gpdf commented Feb 9, 2023

I just noticed this PR. Please hang on before merging - I want to review for the implications for external re-use of pex_config.

@timj
Copy link
Member

timj commented Feb 9, 2023

This PR doesn't bring in a need for any new dependencies. It's moving config code out of pipe_tasks so it can be used more widely.

Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

OK, thanks for waiting. I read through everything and have no concerns.

Copy link
Contributor

@ktlim ktlim left a comment

Choose a reason for hiding this comment

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

I'd think a major feature like this deserves a mention in https://pipelines.lsst.io/v/daily/modules/lsst.pex.config/field-types.html and likely its own page with an example like https://pipelines.lsst.io/v/weekly/modules/lsst.pex.config/registry-intro.html

This transfers the ConfigurableAction code from pipe_tasks to
pex_config on branch tickets/DM-36649-transfer.
Address the review comments surrounding comments, documentation, and
typos.
@natelust natelust merged commit 9c140cc into main Mar 23, 2023
9 checks passed
@natelust natelust deleted the tickets/DM-36649 branch March 23, 2023 13:50
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

6 participants