Skip to content

RFC: refactor internal usage of copy=False to copy=COPY_IF_NEEDED - #16166

Merged
mhvk merged 5 commits into
astropy:mainfrom
neutrinoceros:rfc/internal_copy_if_needed
Mar 10, 2024
Merged

RFC: refactor internal usage of copy=False to copy=COPY_IF_NEEDED#16166
mhvk merged 5 commits into
astropy:mainfrom
neutrinoceros:rfc/internal_copy_if_needed

Conversation

@neutrinoceros

@neutrinoceros neutrinoceros commented Mar 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request will be pushed in 5 layers because we need to run CI 5 times to validate it.
I'll edit this message once it's ready.

Update: actually, 6 times.

This applies the first step described in #16167: making backward compatibility shims introduced in #16142 redundant for internal usage. It is meant to be reviewed commit-by-commit. Particularly, the exit status of devdeps is crucial.

  • commit 1 removes compatibility shims -> devdeps fails (this demonstrate their current necessity)
  • commit 2 reverts commit 1 -> devdeps succeeds anew
  • commit 3 applies internal refactor that should make compatibility shims redundant for internal usage, but doesn't change any API (default values copy=False are preserved) -> devdeps still succeeds
  • commit 4 removes compatibility shims again -> devdeps fails !
  • commit 5 introduces minimal API changes (copy=False becomes copy=COPY_IF_NEEDED in signatures) -> devdeps succeeds once again !
  • commit 6 reverts commit 4

Update again: commit 4 was split out to #16174, but I think keeping the old explanation is maybe less confusing (?) so for now I don't want to edit it further.

By construction, 4 out of 6 commits should cancel out, so squash merging is expected.
Also note that commit 3 is, in itself, a pure refactor, and could be backported (but I don't know that it's wanted). Minor API changes from commit 5 are aimed for astropy 6.1, but could also be rerolled in its own PR if requested (its presence in this PR is supposed to help demonstrating that it's needed in the grand scheme of #16167, but maybe that's only making it more confusing ?).

  • 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.

@github-actions

github-actions Bot commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

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?
  • 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

github-actions Bot commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

👋 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?

@neutrinoceros neutrinoceros mentioned this pull request Mar 6, 2024
Closed
@neutrinoceros
neutrinoceros force-pushed the rfc/internal_copy_if_needed branch from 831d61b to 61d4976 Compare March 6, 2024 09:33
@neutrinoceros
neutrinoceros marked this pull request as ready for review March 6, 2024 10:02
@neutrinoceros
neutrinoceros requested review from a team and taldcroft as code owners March 6, 2024 10:02
@neutrinoceros

neutrinoceros commented Mar 6, 2024

Copy link
Copy Markdown
Contributor Author

A couple missing labels should be added manually:

  • coordinates
  • modeling

@pllim pllim added this to the v6.1.0 milestone Mar 6, 2024
@neutrinoceros

neutrinoceros commented Mar 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Copying from the today's call notes :

Consensus is to keep it simple: Astropy shall match whatever the installed numpy version does. Avoid work to put in and later remove again compatibility layers, configuration options etc.

I'll split out the small signature changes to a separate PR so this one stays a pure refactor and can be merged consensually.
When the refactor part is merged I'll send an update to the dev mailing list to encourage downstream to test it early.

update: the API change was split out into #16174

@neutrinoceros
neutrinoceros force-pushed the rfc/internal_copy_if_needed branch from dd3af2c to 8eb54fa Compare March 7, 2024 17:07
This was referenced Mar 7, 2024
Closed
Merged
pllim
pllim reviewed Mar 8, 2024
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At a glance, this PR looks reasonable. I will give subpackage maintainers some time to review. Thanks!

@pllim pllim added the numpy-dev label Mar 8, 2024
nden
nden approved these changes Mar 9, 2024
View reviewed changes

@nden nden left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes in modeling look good to me.

@neutrinoceros
neutrinoceros force-pushed the rfc/internal_copy_if_needed branch from 8eb54fa to 9aacfd6 Compare March 9, 2024 15:46
@neutrinoceros

neutrinoceros commented Mar 9, 2024

Copy link
Copy Markdown
Contributor Author

I rebased to get around that merge requirements changed after #15603 was merged. As a result, we lost easy access to CI logs that I prepared for reviewers. Hence, I encourage checking out the branch locally and running a fraction of the test suite1 against all commits in the branch if you want to check my story.

Footnotes

  1. running the tests of the subpackage you're reviewing for is probably enough ↩

@neutrinoceros

neutrinoceros commented Mar 10, 2024

Copy link
Copy Markdown
Contributor Author

closing/reopening to retrigger CI now that pytest-doctestplus was updated

@neutrinoceros neutrinoceros reopened this Mar 10, 2024
@neutrinoceros
neutrinoceros force-pushed the rfc/internal_copy_if_needed branch from 9aacfd6 to fc311fc Compare March 10, 2024 07:28
@neutrinoceros
neutrinoceros force-pushed the rfc/internal_copy_if_needed branch from fc311fc to 72aeebe Compare March 10, 2024 13:05
mhvk
mhvk approved these changes Mar 10, 2024
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks all good. Your extra commits effectively allowed you to test that this covers everything, correct?

Is the idea to get just the one commit in as well? That would seem fine to me.

@neutrinoceros

neutrinoceros commented Mar 10, 2024

Copy link
Copy Markdown
Contributor Author

Your extra commits effectively allowed you to test that this covers everything, correct?

that's right. I ran the whole test suite at each step so I'm confident that the interesting commit is complete.

Is the idea to get just the one commit in as well?

Squash-merging should be equivalent to dropping the 4 extra commits, but I'm happy to go with any strategy you like best.

@mhvk

mhvk commented Mar 10, 2024

Copy link
Copy Markdown
Contributor

Sorry, I wasn't clear: is the idea that now we know this works, we merge this one too so that the other two can be rebased on it? (Happy to squash-merge)

@neutrinoceros

neutrinoceros commented Mar 10, 2024

Copy link
Copy Markdown
Contributor Author

Oh, sorry I didn't get that ! yes, that is the plan !

@mhvk
mhvk merged commit 4b727ad into astropy:main Mar 10, 2024
@neutrinoceros
neutrinoceros deleted the rfc/internal_copy_if_needed branch March 10, 2024 20:38
@neutrinoceros neutrinoceros mentioned this pull request May 5, 2024
Merged
1 task
d-giles referenced this pull request in d-giles/astropy-abdufork Jul 26, 2024
@neutrinoceros @d-giles
RFC: refactor internal usage of copy=False to copy=COPY_IF_NEEDED (#1…
81766bf 
…6166)

Now internally we will be OK regardless of how we deal with the numpy 2.0
change in the meaning of `copy`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@nden nden nden approved these changes
@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 Refactoring table time units utils.masked

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@neutrinoceros @mhvk @nden @pllim