Skip to content

FIX: h5py>=3 string decoding - #4893

Merged
mathause merged 8 commits into
pydata:masterfrom
kmuehlbauer:fix-4570
Feb 17, 2021
Merged

FIX: h5py>=3 string decoding#4893
mathause merged 8 commits into
pydata:masterfrom
kmuehlbauer:fix-4570

Conversation

@kmuehlbauer

@kmuehlbauerkmuehlbauer commented Feb 11, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor
  • set decode_vlen_strings=True for h5netcdf backend
  • unpin h5py

This is an attempt to align with h5py=>3.0.0 string decoding changes. Now all strings are read as bytes objects for variable-length strings, or numpy bytes arrays ('S' dtypes) for fixed-length strings [1]. From h5netcdf=0.10.0 kwarg decode_vlen_strings is available. This PR makes use of this to keep backwards compatibility with h5py=2 and conformance with netcdf4-python.

[1] https://docs.h5py.org/en/stable/strings.html

@kmuehlbauer
FIX: set decode_strings=True for h5netcdf backend, convert object s…
814c0eb
…tring to byte string if necessary, unpin h5py
@kmuehlbauerkmuehlbauer changed the title FIX: set decode_strings=True for h5netcdf backend, convert object s…FIX: h5py string decodingFeb 11, 2021
This was referenced Feb 11, 2021
Closed
Closed
Closed
@kmuehlbauer

kmuehlbauer commented Feb 11, 2021

Copy link
Copy Markdown
ContributorAuthor

Nice, I didn't expect to get all green at the first hit.

Nevertheless, please have a thorough look at this. I would also like to know how to document this properly in whats-new.txt. Is this a bug fix or internal change or both?

@dcheriandcherian mentioned this pull request Feb 11, 2021
Closed
6 tasks
shoyer
shoyer reviewed Feb 11, 2021
View reviewed changes
Comment threadxarray/coding/strings.py Outdated
Comment on lines +196 to +197
if arr.dtype.kind == "O":
arr = arr.astype("S1")

@shoyershoyerFeb 11, 2021

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.

Hmm. We should probably just fix this newly introduced issue over in h5netcdf first :)

@kmuehlbauerkmuehlbauerFeb 11, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@shoyer Sure, I'll ping you over there.

@kmuehlbauerkmuehlbauer changed the title FIX: h5py string decodingWIP: FIX: h5py string decodingFeb 11, 2021
@kmuehlbauer

kmuehlbauer commented Feb 11, 2021

Copy link
Copy Markdown
ContributorAuthor

So it was decided to fix this at h5netcdf. I'll update the PR tomorrow.

@kmuehlbauer

kmuehlbauer commented Feb 11, 2021

Copy link
Copy Markdown
ContributorAuthor

All fixes in place, but we need to wait until h5netcdf=0.10.0 is available on conda-forge.

@kmuehlbauerkmuehlbauer changed the title WIP: FIX: h5py string decodingFIX: h5py>=3 string decodingFeb 12, 2021
@kmuehlbauer

kmuehlbauer commented Feb 12, 2021

Copy link
Copy Markdown
ContributorAuthor

This is good to go from my end. Thanks @shoyer for your help to not get lost in this string encoding/decoding maze.

@mathause

mathause commented Feb 16, 2021

Copy link
Copy Markdown
Collaborator

Could you suppress the warning in the test suite? https://github.com/pydata/xarray/pull/4893/checks?check_run_id=1885301619#step:11:278

So the plan is to read strings as bytes in the future?

@kmuehlbauer

kmuehlbauer commented Feb 16, 2021

Copy link
Copy Markdown
ContributorAuthor

So the plan is to read strings as bytes in the future?

That's what h5py does. h5netcdf just added backwards compatibility using that decode_vlen_strings kwarg. Means downstream users can use decode_vlen_strings=True and have same experience for h5py>=3.0.0 and h5py=2.

@kmuehlbauer
FIX: set decode_vlen_strings=True in open classmethod, call remai…
f7a5662
…ning tests with `decode_vlen_strings=True`
@kmuehlbauer

kmuehlbauer commented Feb 16, 2021

Copy link
Copy Markdown
ContributorAuthor

Could you suppress the warning in the test suite? https://github.com/pydata/xarray/pull/4893/checks?check_run_id=1885301619#step:11:278

@mathause The FutureWarnings should be silenced by now. I've set decode_vlen_strings=True in all places. So this should also work after deprecated in h5netcdf.

mathause
mathause approved these changes Feb 16, 2021
View reviewed changes

@mathausemathause left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thanks!

@mathause
mathause merged commit a8ed7ed into pydata:masterFeb 17, 2021
@mathause

mathause commented Feb 17, 2021

Copy link
Copy Markdown
Collaborator

Thanks a lot @kmuehlbauer!

dcherian added a commit to dcherian/xarray that referenced this pull request Feb 17, 2021
@dcherian
Merge remote-tracking branch 'upstream/master' into dataset/quiver
2b1bc32
* upstream/master:
FIX: h5py>=3 string decoding (pydata#4893)
Update matplotlib's canonical (pydata#4919)
Adding vectorized indexing docs (pydata#4711)
Allow fsspec URLs in open_(mf)dataset (pydata#4823)
Fix typos in example notebooks (pydata#4908)
pre-commit autoupdate CI (pydata#4906)
replace the ci-trigger action with a external one (pydata#4905)
Update area_weighted_temperature.ipynb (pydata#4903)
hide the decorator from the test traceback (pydata#4900)
Sort backends (pydata#4886)
Compatibility with dask 2021.02.0 (pydata#4884)
@mikapflmikapfl mentioned this pull request Mar 1, 2021
Merged
3 tasks
@spencerkclarkspencerkclark mentioned this pull request Jan 5, 2022
Merged
@kmuehlbauerkmuehlbauer mentioned this pull request Jan 11, 2022
Open
@soxofaansoxofaan mentioned this pull request Jun 17, 2022
Closed
@kmuehlbauer
kmuehlbauer deleted the fix-4570 branch May 25, 2023 07:12
@kmuehlbauerkmuehlbauer mentioned this pull request May 30, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shoyershoyershoyer left review comments
@mathausemathausemathause 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.

fix compatibility with h5py version 3 and unpin tests

3 participants

@kmuehlbauer@mathause@shoyer