JuliaIO / MAT.jl

Julia module for reading MATLAB files
MIT License
278 stars 71 forks source link

Adapt to changes in HDF5 v0.14 release #145

Closed jmert closed 3 years ago

jmert commented 3 years ago

HDF5.jl v0.14.0 has made a number of significant changes to the interface — this PR updates MAT for those changes.

codecov[bot] commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@0272d7a). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #145   +/-   ##
=========================================
  Coverage          ?   90.26%           
=========================================
  Files             ?        3           
  Lines             ?      565           
  Branches          ?        0           
=========================================
  Hits              ?      510           
  Misses            ?       55           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0272d7a...9103be1. Read the comment docs.

jmert commented 3 years ago

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 @show HDF5.checkvalid(x) statements on parent, dtype, and stype here.

musm commented 3 years ago

Hmm, interesting. Should I hold off on the registration you think?

jmert commented 3 years ago

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?

musm commented 3 years ago

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.

jmert commented 3 years ago

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

dtype is closed/invalid after the GC sweep.