Fix for newer HDMF #1191
Fix for newer HDMF #1191
Conversation
Codecov ReportBase: 89.08% // Head: 89.09% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1191 +/- ##
=======================================
Coverage 89.08% 89.09%
=======================================
Files 76 76
Lines 9448 9451 +3
=======================================
+ Hits 8417 8420 +3
Misses 1031 1031
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| warnings.filterwarnings( | ||
| "ignore", message="HDF5IO was not fully initialized before close" | ||
| ) | ||
| errors = dandi_file(path).get_validation_errors() |
There was a problem hiding this comment.
please add a comment on why needs to be filtered, point to hdmf issue.
PR should be accompanied with the revert of 6d065ec to ensure that we test it with current/buggy hdmf
There was a problem hiding this comment.
Commenting improved in 9ad66c4.
@yarikoptic
As for the second part, sorry if my explanation in the first post was not clear enough, but we specifically do not want to revert that commit. The issue is a real bug in 3.5.0, which we cannot easily circumvent in DANDI. This PR simply leverages the upstream patch, which will become available upon the next release, hopefully one we can ask for right now if you agree this has been properly addressed for our use case (I do).
There was a problem hiding this comment.
Ah, I think I got it. But is this the only place where such warning might be raised? If not, we might need to add it to https://github.com/dandi/dandi-cli/blob/master/tox.ini#L48 instead of to some other tests.
There was a problem hiding this comment.
Well, this is the only test with that issue. Not sure if it's a good idea to move the catching to tox.ini — as that will not be caught when running straight-up pytest... or maybe I'm misunderstanding what you're suggesting? I predominantly use pytest directly to test locally, and none of the tox.ini errors ever popped up 🤔
There was a problem hiding this comment.
In any case upstream said a new patch release for this sounds good.
There was a problem hiding this comment.
confusingly enough pytest reads (as well) config from tox.ini, see https://docs.pytest.org/en/7.1.x/reference/customize.html#tox-ini , that is why you didn't see any error. Ok, if you say that it is the only test -- let's proceed.
|
🚀 PR was released in |
From: hdmf-dev/hdmf#817 (comment)
Short summary (full story in the issue above):
=hdmf-3.5.0breaks ourdandi/tests/test_files.py::test_validate_bogustest. This is because a new deletion method was introduced, which assumes all HDF objects have_HDF5IO__open_links, which some constructors don't create. Upstream has fixed this, but this will only take effect in future versions. However, this will still not fix our test, because upstream would prefer to still raise a warning, which pytest detects as a failure. In order to be able to use future HDMF versions, we need this PR.Since there is no patched HDMF on pypi available for our use, the test suite below doesn't actually test the case which this PR addresses, but I tested it locally with the patched HDMF package. I would propose, if we agree on the fix, we just ping upstream for a new patch release
3.5.1.@yarikoptic