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

deprecate copy_arrays with warning #1797

Merged
merged 16 commits into from
Aug 12, 2024

Conversation

zacharyburnett
Copy link
Member

@zacharyburnett zacharyburnett commented Jul 12, 2024

resolves #1710 #1711

Checklist:

  • pre-commit checks ran successfully
  • tests ran successfully
  • for a public change, a changelog entry was added
  • for a public change, documentation was updated
  • for any new features, unit tests were added

@zacharyburnett zacharyburnett force-pushed the deprecate/copy_arrays branch from 3275907 to c6f9b26 Compare July 15, 2024 13:18
@zacharyburnett zacharyburnett linked an issue Jul 15, 2024 that may be closed by this pull request
@zacharyburnett zacharyburnett changed the title copy_arrays=False -> memmap=False deprecate copy_arrays with warning Jul 15, 2024
@zacharyburnett zacharyburnett force-pushed the deprecate/copy_arrays branch from 231197c to 53383e6 Compare July 15, 2024 16:49
@zacharyburnett zacharyburnett marked this pull request as ready for review July 15, 2024 18:03
@zacharyburnett zacharyburnett requested a review from a team as a code owner July 15, 2024 18:03
@zacharyburnett zacharyburnett self-assigned this Jul 15, 2024
Copy link
Contributor

@braingram braingram left a comment

Choose a reason for hiding this comment

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

I left a few suggestions. One major one being the warning message. Also, if you can think of other ways we can inform users of the upcoming memmap default change that would be great! It's a rather major change and this switch from "copy_arrays" to "memmap" was somewhat intended to provide a warning to the users.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
asdf/_asdf.py Show resolved Hide resolved
asdf/_asdf.py Outdated Show resolved Hide resolved
asdf/_tests/test_deprecated.py Outdated Show resolved Hide resolved
asdf/_tests/test_deprecated.py Show resolved Hide resolved
@braingram
Copy link
Contributor

I also added the Downstream CI label. Some of those packages don't turn warnings into errors (like jwst). Would you check which downstream packages are going to fail (or produce new warnings) with these changes?

@zacharyburnett
Copy link
Member Author

I also added the Downstream CI label. Some of those packages don't turn warnings into errors (like jwst). Would you check which downstream packages are going to fail (or produce new warnings) with these changes?

Sure!

@zacharyburnett zacharyburnett force-pushed the deprecate/copy_arrays branch 2 times, most recently from 28fb0ea to 76588bb Compare July 15, 2024 18:53
@zacharyburnett
Copy link
Member Author

Also, if you can think of other ways we can inform users of the upcoming memmap default change that would be great! It's a rather major change and this switch from "copy_arrays" to "memmap" was somewhat intended to provide a warning to the users.

I tried adding this warning box but GitHub doesn't render the RST correctly:
image

Copy link
Contributor

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Code changes look good to me!

I added a few more suggestions to docs.

The remaining issue is the downstream usage. Of the failed packages most we can update (and perhaps bump asdf to at least 3.1.0 (to use memmap). dkist follows SPEC-0000 and considers asdf a "core" package so we can open a PR which accommodates the older versions (only providing memmap for 3.1.0 and after).

Are there any warnings about this change in the non-failing jobs?

README.rst Outdated Show resolved Hide resolved
asdf/_asdf.py Outdated Show resolved Hide resolved
asdf/_asdf.py Outdated Show resolved Hide resolved
@zacharyburnett
Copy link
Member Author

a few warnings in asdf-astropy and sunpy; I can work on PRs for stdatamodels and roman_datamodels

@braingram
Copy link
Contributor

Thanks!

asdf-astropy is maintained by "us" so that could also use a PR (and gwcs). I also try to make PRs for our other downstream users for stuff like this and am happy to help with PRs. Would you like to divide these up? I think so far we have:

What a kettle of fish! :)

@braingram
Copy link
Contributor

Also one of my suggestions is causing a pre-commit failure.

@zacharyburnett zacharyburnett merged commit 867c280 into asdf-format:main Aug 12, 2024
45 of 47 checks passed
@zacharyburnett zacharyburnett deleted the deprecate/copy_arrays branch August 12, 2024 13:38
@braingram braingram mentioned this pull request Oct 9, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate copy_arrays
2 participants