Adapt to changes in HDF5 v0.14 release #145
Conversation
jmert
force-pushed
the
HDF5_v0.14
branch
2 times, most recently
from
8ca79f9
to
d08e63b
Compare
The corresponding HDF5.read() methods are only defined on the narrower `Union` `DatasetOrAttribute` (`HDF5Object` also includes `HDF5Group`).
a_create -> create_attribute d_create -> create_dataset g_create -> create_group p_create -> create_property a_read -> read_attribute a_write -> write_attribute d_write -> write_attribute attrs -> attributes
This comment has been minimized.
This comment has been minimized.
jmert
force-pushed
the
HDF5_v0.14
branch
2 times, most recently
from
3dd6190
to
314a42b
Compare
jmert
force-pushed
the
HDF5_v0.14
branch
3 times, most recently
from
4ad6b66
to
9103be1
Compare
jmert
changed the title
|
Gah, I've seen the Linux x64/Julia 1.5.3 CI failure (https://github.com/JuliaIO/MAT.jl/runs/1449246460) a couple of times while rebasing this PR, but I cannot reproduce locally and it went away when I temporarily stuck |
|
Hmm, interesting. Should I hold off on the registration you think? |
|
I was hoping not, but it probably deserves a bit of investigation. I'll try running the tests a few more times repeatedly on my Linux machines to see if I can trigger, but I'm afraid it's going to be some subtle thing like somehow a finalizer is running early and could be hard to reliably reproduce. If you have a Linux machine of your own (especially if Ubuntu like CI; mine are Arch), maybe you could try running the tests a few times? |
|
I won't cancel the registration PR since this looks intermittent and could be addressed in a patch release. I'll try to see if I can trigger it, although it would be using Ubuntu through WSL2. |
|
Forcefully running the GC allows me to reproduce the error: diff --git a/src/MAT_HDF5.jl b/src/MAT_HDF5.jl
index 87be6be..e6cf144 100644
--- a/src/MAT_HDF5.jl
+++ b/src/MAT_HDF5.jl
@@ -356,6 +356,10 @@ function m_writearray(parent::HDF5Parent, name::String, adata::AbstractArray{Com
try
stype = dataspace(adata)
if compress
+ GC.gc(true)
+ @show parent
+ @show dtype
+ @show stype
obj_id = create_dataset(parent, name, dtype, stype;
compress = 3, chunk = HDF5.heuristic_chunk(adata))
else
|
By defining `Base.convert(::Type{hid_t}, x)` for all of the high-level
object types, implicit conversion to an integer is permitted.
This has the unfortunate consquence --- as found in JuliaIO/MAT.jl#145
and JuliaIO/MAT.jl#151 --- that incorrectly "wrapping" a wrapped object
was allowed, but doing so allows the garbage collector to reap the
object too early.
The main reason for allowing conversion from an object to integer
(namely `HDF5.hid_t`) is to support passing the objects to `ccall`s
without requiring reaching into the `obj.id` raw integer ID.
We can easily still allow this while disabling implicit conversion by
just replacing all definitions of `Base.convert` with `Base.cconvert`.
Doing this would have caught the mistake in MAT.jl at the Julia type
level rather than having to debug a runtime error.
| matrix: | ||
| version: | ||
| - '1.3' | ||
| - '1' |
There was a problem hiding this comment.
I don't think this actually runs the latest release version (i.e 1.5) (unlike travis)
There was a problem hiding this comment.
I guess it now does. Maybe it was fixed, but I was sure it did not several months ago when transitioning HDF5.jl
There was a problem hiding this comment.
From the CI logs, it looks like it does — it reports downloading Julia v1.5.3: https://github.com/JuliaIO/MAT.jl/pull/145/checks?check_run_id=1449246460#step:3:6
…#750) By defining `Base.convert(::Type{hid_t}, x)` for all of the high-level object types, implicit conversion to an integer is permitted. This has the unfortunate consquence --- as found in JuliaIO/MAT.jl#145 and JuliaIO/MAT.jl#151 --- that incorrectly "wrapping" a wrapped object was allowed, but doing so allows the garbage collector to reap the object too early. The main reason for allowing conversion from an object to integer (namely `HDF5.hid_t`) is to support passing the objects to `ccall`s without requiring reaching into the `obj.id` raw integer ID. We can easily still allow this while disabling implicit conversion by just replacing all definitions of `Base.convert` with `Base.cconvert`. Doing this would have caught the mistake in MAT.jl at the Julia type level rather than having to debug a runtime error.
HDF5.jl v0.14.0 has made a number of significant changes to the interface — this PR updates MAT for those changes.