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

Add pyerfa as core dep and fix imports (fixes devdeps) #7397

Merged
merged 4 commits into from Jan 25, 2024

Conversation

nabobalis
Copy link
Contributor

@nabobalis nabobalis commented Jan 24, 2024

Import has now changed.

@nabobalis nabobalis added Tests Affects tests in some measure Affects Release An issue/bug that affects a released version (use a version label too) Minor Change PR only needs one approval to merge backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 Infrastructure Issues or PRs that affect the CI or packaging of SunPy No Changelog Entry Needed and removed Affects Release An issue/bug that affects a released version (use a version label too) labels Jan 24, 2024
@nabobalis nabobalis marked this pull request as ready for review January 24, 2024 18:58
@nabobalis nabobalis requested review from a team as code owners January 24, 2024 18:58
Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

You have to add pyerfa as a test dependency (even though it should always be there because of astropy).

Given that, you might as well also make the long overdue change of specifying pyerfa as a core dependency and fix the two funny-looking imports in #4343.

@ayshih
Copy link
Member

ayshih commented Jan 24, 2024

("long overdue" because we could have "fixed" those imports once Astropy 4.2 became the minimum dependency)

@nabobalis
Copy link
Contributor Author

Done.

@nabobalis nabobalis changed the title Fix devdeps Add pyefra as core dep and fix imports (fixes devdeps) Jan 24, 2024
@nabobalis nabobalis requested a review from ayshih January 24, 2024 21:24
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

The new dependency should get a changelog entry. Also I'm not sure about backporting a new dependency... can we just ignore the warning on 5.x?

@nabobalis
Copy link
Contributor Author

The new dependency should get a changelog entry. Also I'm not sure about backporting a new dependency... can we just ignore the warning on 5.x?

Hmm, I would say that since pyefra has been a core dependency of astropy for years now, the fact we are adding to ours I don't think changes anything.

I can add the ignore warning to those releases.

I will leave this choice up to you and Albert.

@ayshih
Copy link
Member

ayshih commented Jan 25, 2024

While pyerfa would become a new direct dependency, it has been an indirect dependency since our minimum astropy dependency became 4.2 (i.e., sunpy 3.1, over two years ago). That is, making an indirect dependency a direct dependency has zero effect on anyone with a valid installation of sunpy 3.1+, since their environment is "guaranteed" to have pyerfa present.

My inclination is to go ahead with the backport, because it cleans up the code, but I do think there should be a changelog (trivial), primarily to alert anyone who might have a bizarrely inconsistent environment.

@nabobalis
Copy link
Contributor Author

Rebaesd and added changelog.

@nabobalis nabobalis removed the Minor Change PR only needs one approval to merge label Jan 25, 2024
Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

Can you do me a favor and add it to sunpy-dev-env.yml as well?

@ayshih ayshih changed the title Add pyefra as core dep and fix imports (fixes devdeps) Add pyerfa as core dep and fix imports (fixes devdeps) Jan 25, 2024
@nabobalis
Copy link
Contributor Author

nabobalis commented Jan 25, 2024

Ah I forgot! Sorry.

@nabobalis nabobalis merged commit 3f9d063 into sunpy:main Jan 25, 2024
26 of 27 checks passed
Copy link

lumberbot-app bot commented Jan 25, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 5.0
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 3f9d063adae5bd36be6df2a0712b47b829a26119
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #7397: Add pyerfa as core dep and fix imports (fixes devdeps)'
  1. Push to a named branch:
git push YOURFORK 5.0:auto-backport-of-pr-7397-on-5.0
  1. Create a PR against branch 5.0, I would have named this PR:

"Backport PR #7397 on branch 5.0 (Add pyerfa as core dep and fix imports (fixes devdeps))"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app lumberbot-app bot added the Still Needs Manual Backport This PR needs manually backporting label Jan 25, 2024
Copy link

lumberbot-app bot commented Jan 25, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 5.1
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 3f9d063adae5bd36be6df2a0712b47b829a26119
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #7397: Add pyerfa as core dep and fix imports (fixes devdeps)'
  1. Push to a named branch:
git push YOURFORK 5.1:auto-backport-of-pr-7397-on-5.1
  1. Create a PR against branch 5.1, I would have named this PR:

"Backport PR #7397 on branch 5.1 (Add pyerfa as core dep and fix imports (fixes devdeps))"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@nabobalis nabobalis deleted the devdeps branch January 25, 2024 19:25
@nabobalis nabobalis removed the Still Needs Manual Backport This PR needs manually backporting label Jan 25, 2024
nabobalis added a commit that referenced this pull request Jan 25, 2024
[5.1] Add pyerfa as core dep and fix imports (fixes devdeps) #7397
nabobalis added a commit that referenced this pull request Jan 25, 2024
[5.0] Add pyerfa as core dep and fix imports (fixes devdeps) #7397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 5.0 on-merge: backport to 5.0 backport 5.1 on-merge: backport to 5.1 Infrastructure Issues or PRs that affect the CI or packaging of SunPy Tests Affects tests in some measure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants