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

Solve some h5py deprecation warnings #2874

Merged

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Aug 17, 2020

PR Summary

h5py deprecated their dataset.value attribute and now recommend using dataset[()] instead.
The change was already performed in most of the code, this PR adresses the remaining occurrences.

PR Checklist

  • pass black --check yt/
  • pass isort . --check --diff
  • pass flake8 yt/
  • pass flynt yt/ --fail-on-change --dry-run -e yt/extern
  • add tests (a new GH workflow)

@neutrinoceros neutrinoceros added yt-4.0 feature targeted for the yt-4.0 release refactor improve readability, maintainability, modularity labels Aug 17, 2020
@neutrinoceros neutrinoceros marked this pull request as draft August 17, 2020 08:03
@neutrinoceros
Copy link
Member Author

making this a draft for now. I'll check in the test logs if I've missed any occurrences.

@neutrinoceros
Copy link
Member Author

There is still one deprecation warning

H5pyDeprecationWarning: The default file mode will change to 'r' (read-only) in h5py 3.0. To suppress this warning, pass the mode you need to h5py.File(), or set the global default h5.get_config().default_file_mode, or set the environment variable H5PY_DEFAULT_READONLY=1. Available modes are: 'r', 'r+', 'w', 'w-'/'x', 'a'. See the docs for details.

But I couldn't locate the problematic h5py.File() call, so I reapplied the script from #2623 to make it a bit easier to discover with regex (still failed).

@neutrinoceros neutrinoceros marked this pull request as ready for review August 17, 2020 17:24
@munkm
Copy link
Member

munkm commented Aug 17, 2020

But I couldn't locate the problematic h5py.File() call,

Is it the handler in the OpenPMD frontend? It doesn't explicitly seem to use the mode anywhere that I see.

@munkm
Copy link
Member

munkm commented Aug 17, 2020

Also lines 97 and 146 of yt/geometry/geometry_handler.py don't use mode=.

@neutrinoceros
Copy link
Member Author

Is it the handler in the OpenPMD frontend? It doesn't explicitly seem to use the mode anywhere that I see.

I still can't find it... but because the data_structures.py file imports h5py as h5, I wasn't able to auto-match it. Nice catch !

Also lines 97 and 146 of yt/geometry/geometry_handler.py don't use mode=.

right ! they also weren't matched by my regex because the mode argument is passed as a variable rather than a plain string, well played !

I just fixed those, but I would really like to know where exactly it the remaining outcast !

@munkm
Copy link
Member

munkm commented Aug 17, 2020

I just fixed those, but I would really like to know where exactly it the remaining outcast !

Oh! None of those fixed the warning???

@neutrinoceros
Copy link
Member Author

I mean I just fixed them right now because I didn't catch them before you pointed them out !
So... maybe that did it actually. Wait and see ! :)

@neutrinoceros
Copy link
Member Author

there's still one in the log, but I really can't find it's origin

ProjectionValues_DD0011_all_('enzo', 'Density')_1_density ... H5pyDeprecationWarning: The default file mode will change to 'r' (read-only) in h5py 3.0. To suppress this warning, pass the mode you need to h5py.File(), or set the global default h5.get_config().default_file_mode, or set the environment variable H5PY_DEFAULT_READONLY=1. Available modes are: 'r', 'r+', 'w', 'w-'/'x', 'a'. See the docs for details.

Is it the handler in the OpenPMD frontend? It doesn't explicitly seem to use the mode anywhere that I see.

@munkm I've been trying to locate but if you're referring to this line

        self._handle = HDF5FileHandler(filename)

that's not it: this calls h5py.File(filename, mode="r") underneath.

@neutrinoceros neutrinoceros force-pushed the address_h5py_deprecation_warnings branch from d797531 to d97d9c1 Compare August 18, 2020 11:50
@munkm
Copy link
Member

munkm commented Aug 18, 2020

I've been trying to locate

You already fixed the instances I was talking about with h5 in OpenPMD . 🙂

There are a couple of other instances where h5py is imported as h5 -- I bet the culprit is one of those. They all use h5.File() without mode=

The files I see that do that import are:

  • yt/utilities/grid_data_format/conversion/conversion_athena.py
  • yt/utilities/grid_data_format/tests/test_writer.py
  • yt/utilities/minimal_representation.py

@neutrinoceros
Copy link
Member Author

nice catch, I'm on it

@neutrinoceros
Copy link
Member Author

done ! This allowed me to find the one call that didn't have a "mode" argument at all (hence wasn't future proof). Thanks a lot for your assistance with this !

@munkm
Copy link
Member

munkm commented Aug 18, 2020

Wooo!!! ... With our powers combined! 😉

@neutrinoceros
Copy link
Member Author

I've added a workflow to catch those on future pull requests !
this catches import _h5py as h5 as errors (because as we saw, those hurt discoverability), and calls to h5py.FIle() without a second argument. I've checked on my fork that the workflow correctly detect the problems I'm fixing here 🎉

@neutrinoceros
Copy link
Member Author

Welp. I had to update a file opening mode (previously implicit) from "w" to "r" in order to pass tests.
specifically, the test involved here is
yt/utilities/tests/test_minimal_representation.py::test_store

However I have serious doubts that this is the intended behaviour here since the affected method is named store, it really sounds to me like it's supposed to be writing stuff. The problem is that opening with mode="w" fails because the storage file is apparently already open (in "r" mode, I guess ?), but I wasn't able to track down completely when it's done. I lost track of it here

return self.region(c, self.domain_left_edge, self.domain_right_edge, **kwargs)

and I don't get where/how self.region is being defined

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

seems good to me, and I think adding more stuff to the rules will be fine over time

@matthewturk matthewturk merged commit 6dbdfc3 into yt-project:master Sep 14, 2020
@neutrinoceros neutrinoceros deleted the address_h5py_deprecation_warnings branch September 14, 2020 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor improve readability, maintainability, modularity yt-4.0 feature targeted for the yt-4.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants