Skip to content

Fix #2812: Drop HDF5 support #3138

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix #2812: Drop HDF5 support #3138

wants to merge 7 commits into from

Conversation

hossam26644
Copy link
Contributor

No description provided.

@hossam26644
Copy link
Contributor Author

@benjeffery please give it a look; make sure that I did not leave unnecessary tests (that do not fail)

@hossam26644 hossam26644 marked this pull request as ready for review April 10, 2025 15:04
@hossam26644
Copy link
Contributor Author

@benjeffery I also updated the license year in some files to 2025 (maybe it should have been a separate PR?)

@codecov Codecov
Copy link

codecov bot commented Apr 10, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.58%. Comparing base (feeecb5) to head (ac6ddfb).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3138      +/-   ##
==========================================
- Coverage   89.94%   89.58%   -0.37%     
==========================================
  Files          29       28       -1     
  Lines       32651    31841     -810     
  Branches     5854     5849       -5     
==========================================
- Hits        29368    28524     -844     
- Misses       1869     1887      +18     
- Partials     1414     1430      +16
Flag Coverage Δ
c-tests 86.66% <ø> (ø)
lwt-tests 80.38% <ø> (-0.40%) ⬇️
python-c-tests 88.14% <ø> (-1.09%) ⬇️
python-tests 98.80% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
c/tskit/core.c 95.50% <ø> (ø)
c/tskit/core.h 100.00% <ø> (ø)
python/_tskitmodule.c 88.14% <ø> (-1.09%) ⬇️
python/tskit/__init__.py 100.00% <ø> (ø)
python/tskit/cli.py 96.66% <ø> (-0.28%) ⬇️
python/tskit/util.py 99.25% <ø> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

benjeffery
benjeffery requested changes Apr 10, 2025
View reviewed changes
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

Looks good!
Can we make the changelog line a bit more descriptive to include a mention of "legacy formats from msprime version blah"

The metadata tests shouldn't be removed, just remove the HDF5 from the name.

jeromekelleher
jeromekelleher reviewed Apr 11, 2025
View reviewed changes
Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks good. We should do a search for h5py across the full repo to make sure we've cleared it out from all the requirements files etc as well.

python/tskit/util.py Outdated
@@ -867,7 +867,7 @@ def raise_known_file_format_errors(open_file, existing_exception):
"The specified file appears to be in HDF5 format. This file "
"may have been generated by msprime < 0.6.0 (June 2018) which "
"can no longer be read directly. Please convert to the new "
"kastore format using the ``tskit upgrade`` command."
"kastore format using the ``tskit upgrade`` command from tskit version 0.6.2"
Copy link
Member

Choose a reason for hiding this comment

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

Have we removed the "upgrade" CLI then also? I'm not sure what this would do now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, yes. Now tskit upgrade is totally removed

python/CHANGELOG.rst Outdated
@@ -7,6 +7,9 @@
- Meatdata.schema was returning a modified schema, this is fixed to return a copy of
the original schema instead (:user:`benjeffery`, :issue:`3129`, :pr:`3130`)

**Breaking Changes**
- HDF5 support is dropped (:user:`hossam26644`, :issue:`2812`, :pr:`3138`)
Copy link
Member

Choose a reason for hiding this comment

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

Something a bit more descriptive might be helpful here.

@hossam26644
Copy link
Contributor Author

@benjeffery Ok, I put back the metadataroundtrip testing. It was not converting to and from HDF5 using the tskit apis; was it actually doing any conversions?

@benjeffery
Copy link
Member

@benjeffery Ok, I put back the metadataroundtrip testing. It was not converting to and from HDF5 using the tskit apis; was it actually doing any conversions?

No, the name was a relic from when the file format was HDF5 only.

@hossam26644
Copy link
Contributor Author

@benjeffery ready for another review round

@benjeffery
Copy link
Member

Almost there, we need to remove h5py from all requirements files.

@jeromekelleher
Copy link
Member

Nice, shedding a bunch of baggage here!

@benjeffery
Copy link
Member

Great! Just a squash now and we should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromekelleher jeromekelleher jeromekelleher left review comments

@benjeffery benjeffery benjeffery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hossam26644 @benjeffery @jeromekelleher