Skip to content

TST: Fix devdeps#15363

Merged
pllim merged 2 commits into
astropy:mainfrom
pllim:ugh-devdeps-you-killing-me
Sep 21, 2023
Merged

TST: Fix devdeps#15363
pllim merged 2 commits into
astropy:mainfrom
pllim:ugh-devdeps-you-killing-me

Conversation

@pllim
Copy link
Copy Markdown
Member

@pllim pllim commented Sep 21, 2023 •
edited
Loading

Description

This pull request is to address:

I don't think it'll be a clean backport but I do want to backport some of this to v5.3.x to prep for Python 3.12 work. No, wait, never mind. We already pinned numpy<2 in v5.3.x so none of this is relevant.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@pllim
TST: Fix devdeps not pulling in numpy 2.0.dev
69598ce 
because matplotlib was pulling in contourpy that pinned numpy<2.

TST: Fix mpldev not pulling in from Scientific Python nightlies.

TST: Remove asdf-astropy dev testing completely because it is downstream now.

TST: Add verbose to devdeps to help debugging.

TST: Removed redundant extras directive for devdeps. It is always called with test.
@pllim
MNT: np.chararray is now np.char.chararray
1853cda 
as per numpy 2.0
@pllim pllim added this to the v5.3.4 milestone Sep 21, 2023
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 21, 2023

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 21, 2023

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@pllim pllim modified the milestones: v5.3.4, v6.0 Sep 21, 2023
This was referenced Sep 21, 2023
Merged
Merged
mhvk
mhvk reviewed Sep 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

This looks good! I think separating out mpldev is the way to go for now. But should it perhaps be part of cron rather than regular CI? Most PRs do nothing with matplotlib.

Comment thread astropy/table/column.py
def tolist(self):
if self.dtype.kind == "S":
return np.chararray.decode(self, encoding="utf-8").tolist()
return np.char.chararray.decode(self, encoding="utf-8").tolist()
Copy link
Copy Markdown
Contributor

@mhvk mhvk Sep 21, 2023

Choose a reason for hiding this comment

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

Wow, that certainly is simpler than I thought. Possibly, eventually np.char will also be deprecated, but we can take it one at a time!

Copy link
Copy Markdown
Contributor

@saimn saimn Sep 21, 2023

Choose a reason for hiding this comment

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

This may be painful for io.fits 😱

Copy link
Copy Markdown
Contributor

@mhvk mhvk Sep 21, 2023

Choose a reason for hiding this comment

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

At least for now it seems np.char continues to exist. I think it is meant to go to a "legacy" grouping (see https://numpy.org/neps/nep-0052-python-api-cleanup.html#cleaning-up-the-submodule-structure). I had missed this, but fortunately @pllim just tested things and realized that it was just np.chararray that is the problem - it will be removed from the main namespace.

Copy link
Copy Markdown
Member Author

@pllim pllim Sep 21, 2023

Choose a reason for hiding this comment

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

Do you wanna think about future-proofing and remove usage of np.char altogether? 😬

Copy link
Copy Markdown
Contributor

@mhvk mhvk Sep 21, 2023

Choose a reason for hiding this comment

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

Maybe. Though I just noticed that there is a current PR over at numpy trying to speed it up by converting some of the code to C. So, maybe best to leave things as they are for now -- especially as we're dealing with so much churn already!

@pllim
Copy link
Copy Markdown
Member Author

pllim commented Sep 21, 2023

But should it perhaps be part of cron rather than regular CI? Most PRs do nothing with matplotlib.

Unfortunately, wcsaxes does get broken a lot by matplotlib, so I am not sure if I am comfortable with moving it to cron.

@pllim pllim marked this pull request as ready for review September 21, 2023 14:35
@pllim pllim requested a review from taldcroft as a code owner September 21, 2023 14:35
mhvk
mhvk approved these changes Sep 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk 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 how it helps to have regular PRs have failures for it -- while I can see that it is useful to ensure we don't introduce problems for numpy-dev. But really you are the one who knows this best, so happy to approve!

@pllim
Copy link
Copy Markdown
Member Author

pllim commented Sep 21, 2023 •
edited
Loading

The goal of this PR is to kinda do what we have been doing anyway. So if you think mpldev should be in cron, then really it is a separate PR. But let's see if it is even a problem first. It actually completed way faster than numpy-dev job (because the mpldev job does not pull remote data).

@mhvk
Copy link
Copy Markdown
Contributor

mhvk commented Sep 21, 2023

OK, makes sense!

@pllim pllim merged commit 7ca3366 into astropy:main Sep 21, 2023
@pllim pllim deleted the ugh-devdeps-you-killing-me branch September 21, 2023 15:09
@pllim pllim mentioned this pull request Feb 14, 2024
Closed
@neutrinoceros neutrinoceros mentioned this pull request Feb 28, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saimn saimn saimn left review comments

@mhvk mhvk mhvk approved these changes

@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft is a code owner

Assignees

No one assigned

Labels

no-changelog-entry-needed numpy-dev table testing

Projects

None yet

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pllim @mhvk @saimn