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

removes incorrect block indices from asdf test data #7145

Merged
merged 2 commits into from Aug 17, 2023

Conversation

braingram
Copy link
Contributor

PR Description

Asdf is making some major updates to internal block reading and writing code (see this PR for the extensive changes).

One of the changes includes issuing a new warning when a file is opened which contains an incorrect block index. The block index is a small, optional YAML document at the end of an ASDF file that contains the offsets of each ASDF binary block within the ASDF file (see the asdf-standard docs for more details). Hand editing ASDF files can often lead to incorrect offsets in the block index. When asdf detects a block index that is incorrect (or if no block index is found) it ignores the block index and falls back to reading blocks sequentially starting with the first.

With sunpy main tested against the above asdf PR several tests are failing (see here for the full log):

FAILED sunpy/sunpy/io/special/asdf/tests/test_genericmap.py::test_load_100_file_with_shift - asdf.exceptions.AsdfBlockIndexWarning: Invalid block index contents for block 0, fal...
FAILED sunpy/sunpy/io/special/asdf/tests/test_genericmap.py::test_load_100_file_with_no_shift - asdf.exceptions.AsdfBlockIndexWarning: Invalid block index contents for block 0, ...
==================================================================== 2 failed, 161 passed, 4 skipped in 58.33s 

This is due to 2 test files in the sunpy repository that contain block index offsets that are incorrect (see the changed files in this PR).

This PR deletes the invalid block index from each of these files.

@braingram
Copy link
Contributor Author

braingram commented Aug 14, 2023

It appears that pre-commit is modifying the data files. Perhaps .pre-commit-config.yaml could use an exclude line?:

exclude: "\\.asdf$"

EDIT: edited regex to better match just files with .asdf extension

@nabobalis
Copy link
Contributor

Yeah, we need to add that to the precommit

@braingram
Copy link
Contributor Author

Thanks! Would you like me to sneak that in here?

@nabobalis
Copy link
Contributor

Yes please!

@nabobalis nabobalis requested a review from Cadair August 14, 2023 21:24
@nabobalis nabobalis added Minor Change PR only needs one approval to merge backport 5.0 on-merge: backport to 5.0 io/ASDF Issues with ASDF No Changelog Entry Needed labels Aug 17, 2023
@nabobalis nabobalis removed the request for review from Cadair August 17, 2023 18:20
@nabobalis nabobalis added the Run cron CI Run cron CI on this PR label Aug 17, 2023
@nabobalis nabobalis closed this Aug 17, 2023
@nabobalis nabobalis reopened this Aug 17, 2023
@nabobalis
Copy link
Contributor

I wanted to run the devdeps test that requires a label plus reopening a PR. Sorry for the trouble.

@nabobalis nabobalis merged commit ec832c2 into sunpy:main Aug 17, 2023
25 of 39 checks passed
@nabobalis
Copy link
Contributor

Thank you @braingram

meeseeksmachine pushed a commit to meeseeksmachine/sunpy that referenced this pull request Aug 17, 2023
@braingram braingram deleted the asdf_bad_block_indices branch August 17, 2023 19:03
@braingram
Copy link
Contributor Author

Thanks! @nabobalis

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 io/ASDF Issues with ASDF Minor Change PR only needs one approval to merge No Changelog Entry Needed Run cron CI Run cron CI on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants