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

Add support for nc_set_alignment and nc_get_alignment #1183

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

hmaarrfk
Copy link
Contributor

These functions were added in 4.9.0 in Unidata/netcdf-c#2206

I'm not sure how you want to handle the error cases. Feedback is appreciated.

@jswhit
Copy link
Collaborator

jswhit commented Aug 26, 2022

a typo is causing the tests to fail

@hmaarrfk
Copy link
Contributor Author

do you want some kind of test here?

@hmaarrfk
Copy link
Contributor Author

if you can point me to where to write it, i'm glad to write one, I've written similar ones in our libraries that consume netcdf4

@jswhit
Copy link
Collaborator

jswhit commented Aug 26, 2022

a test would be nice yes - you can perhaps just set the alignment, close the file and re-open, check the alignment matches what you expect. example at test/tst_shape.py

@jswhit
Copy link
Collaborator

jswhit commented Aug 26, 2022

also please add a Changelog entry

@hmaarrfk
Copy link
Contributor Author

This "passes" locally, but only in the sense that I don't know how to run the test with the netcdf4 API only.

I have written the test with the h5py API as a placeholder.

Honestly, I'm not sure netcdf4 exposes the required features of the hdf5 API. So maybe we can remove the assert statements at the end of the test?

@hmaarrfk
Copy link
Contributor Author

one more time.

@jswhit
Copy link
Collaborator

jswhit commented Aug 30, 2022

still failing to import h5py, and import of set_alignment failing with netcdf-c 4.8.1

@hmaarrfk
Copy link
Contributor Author

still failing to import h5py,

I'm not sure how best to handle this. I could skip these tests on your CI, but this is the only way, to my knowledge, to test the HDF5 alignment.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Aug 30, 2022

Ok new tests will fail with something like:l if alignment isn't set (i removed the set alignment call from the test setup to generate this error message)

            for key, address in addresses.items():
                is_aligned = (address % 4096) == 0
>               assert is_aligned, f"{key} is not aligned. Address = 0x{address:x}"
E               AssertionError: data00 is not aligned. Address = 0x1800
E               assert False

test/tst_alignment.py:132: AssertionError

using only h5ls and not h5py

@hmaarrfk
Copy link
Contributor Author

And with 4.8.1:

 pytest test/tst_alignment.py --verbose
================================================= test session starts ==================================================
platform linux -- Python 3.9.13, pytest-7.1.2, pluggy-1.0.0 -- /home/mark/mambaforge/envs/nc/bin/python
cachedir: .pytest_cache
rootdir: /home/mark/git/netcdf4-python
collected 2 items

test/tst_alignment.py::AlignmentTestCase::test_setting_alignment SKIPPED (No support for set_alignment in li...) [ 50%]
test/tst_alignment.py::AlignmentTestCase::test_version_settings PASSED                                           [100%]

============================================= 1 passed, 1 skipped in 0.09s =============================================

@jswhit
Copy link
Collaborator

jswhit commented Aug 30, 2022

you can leave the h5py test but we will have to update the github actions workflows to install h5py

@hmaarrfk
Copy link
Contributor Author

you can leave the h5py test but we will have to update the github actions workflows to install h5py

Correct. I can likely find how to do that.

I can also "skip" those tests if h5ls isn't found. It is typically hard to find binary tools when installed through pip.

Do you want me to skip the test, or fail the test if h5py isn't installed/

@jswhit
Copy link
Collaborator

jswhit commented Aug 30, 2022

hdf5 is already as an ubuntu package, so there a good chance h5ls will be found. But, if not skipping that test with a warning is fine.

EDIT: I see you are right, and hdf5 is installed via conda for some of the workflows. But using h5ls instead of h5py is preferable if it works, since it doesn't add another dependency.

@hmaarrfk
Copy link
Contributor Author

So conda(-forge) will install the binary tools for hdf5 when one install the hdf5 package.

However, ubuntu will require you to install hdf5-tools:

$ h5ls
Command 'h5ls' not found, but can be installed with:
sudo apt install hdf5-tools

Ultimately, I need some other kind of dependency to run this test.

I can add the tools to ubuntu, we can see how far the tests go.

I am weary to add yet an other (mandatory) test dependency without documenting it thoroughly.

@hmaarrfk
Copy link
Contributor Author

i'm happy to make the dependency optional after we test this one last round with your CI (it is hard to tell exactly what tests are skipped without running tests in verbose mode)

@jswhit
Copy link
Collaborator

jswhit commented Aug 31, 2022

just adding to the ubuntu test should be sufficient (and have the test be skipped if h5ls is not available)

@hmaarrfk
Copy link
Contributor Author

Sigh, I don't really understand why this is failing. Maybe some strange MPICH bug. I'm investigating hmaarrfk#1 no need to bother restarting the CIs all the time.

@hmaarrfk hmaarrfk marked this pull request as draft August 31, 2022 05:23
@hmaarrfk hmaarrfk force-pushed the add_set_alignment branch 3 times, most recently from 6c5b29a to 015477e Compare August 31, 2022 05:59
@hmaarrfk
Copy link
Contributor Author

ok, i believe i've fixed most tests, but windows seems to be failing even without this PR:hmaarrfk#2

@jswhit
Copy link
Collaborator

jswhit commented Aug 31, 2022

windows tests are failing on master right now too, so that's not your problem.

@jswhit
Copy link
Collaborator

jswhit commented Aug 31, 2022

Looks good now @hmaarrfk - thanks for seeing this through. Will the tests starting failing when conda updates to netcdf-c 4.9.0 (since the tests will no longer be skipped and h5ls may not be found)?

@hmaarrfk
Copy link
Contributor Author

jswhit:

  • On conda forge, libnetcdf depend on hdf5 which includes all the binary packages for now. if this changes, we could add an explicit dependency on hdf5-bin later.
  • My goal was to remove the addition of hdf5-tools as a dependency on ubuntu, and make sure the tests are skipped there.

@jswhit
Copy link
Collaborator

jswhit commented Aug 31, 2022

@hmaarrfk I can merge now if you think this is done

test/tst_alignment.py Outdated Show resolved Hide resolved
test/tst_alignment.py Outdated Show resolved Hide resolved
test/tst_alignment.py Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor Author

1 more change ;)

@hmaarrfk
Copy link
Contributor Author

Ok i added the test skipping logic if h5ls is not found. less than "ideal" but for this tiny feature, I think it is ok.

For what its worth, even if h5ls is not found, calls to set_alignment and get_alignment are side effects (because it was implemented as a global state in netcdf-c) will still be felt by downstream calls.

Before merging, I would like to remove the two commits where I change, and then unchange, your ci files.

Do you want me to remove fail-fast: false?

@hmaarrfk
Copy link
Contributor Author

0001-Disable-fail-fast.patch.txt

Patch to disable fail fast

@hmaarrfk
Copy link
Contributor Author

If you are happy with this state, then feel free to merge.

@hmaarrfk hmaarrfk marked this pull request as ready for review August 31, 2022 15:08
@hmaarrfk
Copy link
Contributor Author

FYI: locally i deleted the h5ls executable from my environment and saw:

10:56 $ pytest test/tst_alignment.py --verbose
================================================= test session starts ==================================================
platform linux -- Python 3.9.13, pytest-7.1.2, pluggy-1.0.0 -- /home/mark/mambaforge/envs/nc/bin/python
cachedir: .pytest_cache
rootdir: /home/mark/git/netcdf4-python
collected 2 items

test/tst_alignment.py::AlignmentTestCase::test_setting_alignment SKIPPED (h5ls not found.)                       [ 50%]
test/tst_alignment.py::AlignmentTestCase::test_version_settings PASSED                                           [100%]

============================================= 1 passed, 1 skipped in 0.08s =============================================

@jswhit jswhit merged commit 065ba17 into Unidata:master Aug 31, 2022
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