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

replace usages of copy_arrays with memmap for asdf>=3.1.0 #230

Merged
merged 5 commits into from
Jul 23, 2024

Conversation

zacharyburnett
Copy link
Contributor

follow-on to asdf-format/asdf#1797

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.90%. Comparing base (f047a6b) to head (e686404).
Report is 23 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #230   +/-   ##
=======================================
  Coverage   97.90%   97.90%           
=======================================
  Files          54       54           
  Lines        2050     2055    +5     
=======================================
+ Hits         2007     2012    +5     
  Misses         43       43           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zacharyburnett zacharyburnett marked this pull request as ready for review July 18, 2024 15:15
@braingram
Copy link
Contributor

Hmmm... the oldestdeps should have failed here since asdf==2.13 (our minimum) doesn't have memmap:

"asdf>=2.13",

It doesn't seem to even be running minimum_dependencies :-/ and is picking up asdf 3.3.0
https://github.com/astropy/asdf-astropy/actions/runs/9993708226/job/27621947749?pr=230#step:10:24

@zacharyburnett what do you think we should do here? The usage is only in tests so run-time asdf-astropy doesn't need asdf>=3.1.0 but if we fix our oldest deps it will start failing (since it uses the runtime requirements). The tests would also fail for anyone who packages this repository using the run-time requirements to run tests (like the asdf issue that came up yesterday). We could update the tests to check the asdf version (and use either copy_arrays or memmap) but I'm not sure if it's really worth it (although that would allow us to fix the oldestdeps and hopefully have it not fail).

@zacharyburnett
Copy link
Contributor Author

Hmmm... the oldestdeps should have failed here since asdf==2.13 (our minimum) doesn't have memmap:

"asdf>=2.13",

It doesn't seem to even be running minimum_dependencies :-/ and is picking up asdf 3.3.0
https://github.com/astropy/asdf-astropy/actions/runs/9993708226/job/27621947749?pr=230#step:10:24
@zacharyburnett what do you think we should do here? The usage is only in tests so run-time asdf-astropy doesn't need asdf>=3.1.0 but if we fix our oldest deps it will start failing (since it uses the runtime requirements). The tests would also fail for anyone who packages this repository using the run-time requirements to run tests (like the asdf issue that came up yesterday). We could update the tests to check the asdf version (and use either copy_arrays or memmap) but I'm not sure if it's really worth it (although that would allow us to fix the oldestdeps and hopefully have it not fail).

I think the only two options we have that maintain green status are

  • remove oldestdeps testing or
  • use copy_arrays or memmap based on version

I'm inclined toward the latter.

@zacharyburnett zacharyburnett force-pushed the deprecate/copy_arrays branch 2 times, most recently from 4edd2d9 to d69eb02 Compare July 18, 2024 19:45
@zacharyburnett zacharyburnett changed the title replace usages of copy_arrays with memmap replace usages of copy_arrays with memmap for asdf>=3.1.0 Jul 18, 2024
@braingram
Copy link
Contributor

@zacharyburnett would you rebase this to (hopefully) allow the oldestdeps to use the oldest versions (now that #232 is in).

Also pre-commit is complaining of a RET505 error.
https://results.pre-commit.ci/run/github/271820376/1721656013.8blo7lrzSCa9pRAmDI-kog

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.

Thanks!

@braingram
Copy link
Contributor

Oh, looks like we don't have branch protection rules 😮 Let's wait for the CI to go green before merging.

@braingram braingram merged commit 9348f89 into astropy:main Jul 23, 2024
31 checks passed
@zacharyburnett zacharyburnett deleted the deprecate/copy_arrays branch July 23, 2024 16:53
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.

2 participants