Fix tuple types for 0.4 #232

rened · 2015-04-24T07:51:06Z

Continuing here from #228 to allow for collaborative editing.

fundamental · 2015-04-24T15:23:44Z

@rened if you need a hand getting this to work let me know where to start modifying as this issue is a blocker for me as I need to get at my data to start debugging/replicating a gc bug somewhere deep inside the base julia.

rened · 2015-04-24T15:31:41Z

@fundamental Thanks! I am waiting to see whether Jeff's recent fix now works for @simonster

simonster · 2015-04-24T18:54:51Z

src/jld_types.jl

-    !isa(T, (Union(Tuple, DataType)...)) && unknown_type_err(T)
-    T = T::(Union(Tuple, DataType)...)
+function h5type(parent::JldFile, T::TupleType, commit::Bool)
+    @assert isa(T, TupleType)


This is supposed to be a no-op, right? Seeing as this is asserting, it seems like the wrong method is getting dispatched...

simonster · 2015-04-24T18:56:34Z

Jeff's fix helped a bit, but I think the underlying issue is actually a dispatch bug that I need to track down, and that was just something I noticed while trying to work around it.

simonster · 2015-04-24T18:59:12Z

src/jld_types.jl

-isopaque(t::DataType) = isempty(t.names)
+isopaque(t::TupleType) = t == EMPTY_TUPLE_TYPE
+# isopaque(t::DataType) = isempty(fieldnames(t))
+isopaque(t::DataType) = isa(t, TupleType) ? t == EMPTY_TUPLE_TYPE : isempty(fieldnames(t))


This is how I noticed JuliaLang/julia#10978, but I think isopaque(t::TupleType) should be called if isa(t, TupleType), and not this method.

Do we still need this?

The tests still fail without it, so the answer seems to be yes, although I'm not sure why.

simonster · 2015-04-24T19:23:01Z

Filed JuliaLang/julia#10995 with test case

simonster · 2015-04-24T22:00:25Z

Filed JuliaLang/julia#10998. This one I think I can work around.

sebastiang · 2015-04-24T22:57:44Z

Wish I'd looked earlier to see this work going on! Was hacking away at it a bit myself. Let me know if you need any help. I just got to the SimpleVector issue myself, but perhaps you have a workaround? It caused me to file JuliaLang/julia#10999

simonster · 2015-04-24T23:11:20Z

The SimpleVector issue is not really a bug, just an implementation change. It was among the easier things to handle.

The PR here currently passes the test suite (although not the JLDArchives tests yet). However, you probably don't want to use it yet because I can't guarantee that any files you save with it will be readable by when this is in a state to be merged to master. Anything not involving tuples or types with fields that are tuples is probably okay, but anything else might not work. I also need to add more backwards compatibility tests to JLDArchives to see how badly this breaks reading older files, and fix that as best as I can.

sebastiang · 2015-04-24T23:20:54Z

Understood! I have an aversion to magical reflection based serialization in any language, so was just planning on using the raw HDF5 interfaces mostly anyway.

coveralls · 2015-04-25T05:17:38Z

Changes Unknown when pulling e7e0594 on tuplefix into * on master*.

coveralls · 2015-04-25T05:17:38Z

Changes Unknown when pulling e7e0594 on tuplefix into * on master*.

coveralls · 2015-04-25T05:54:06Z

Changes Unknown when pulling ad00d79 on tuplefix into * on master*.

coveralls · 2015-04-25T06:04:17Z

Changes Unknown when pulling ad00d79 on tuplefix into * on master*.

timholy · 2015-04-25T08:54:20Z

Thanks a bunch for this, everyone! I'll also link JuliaLang/julia#10999 (comment) as a general issue of growing concern to me (but without me yet having carved out the time to find a general solution solution).

simonster · 2015-04-25T14:35:19Z

I added test files to JLDArchives that have our entire test suite saved with 0.3 and master. Those tests all pass (files written with both versions can be read by both versions), except for the SubArray tests, which fail because the structure of the type has changed. (Maybe we should reconstruct the type in this case too instead of throwing an error.)

This doesn't mean everything works, because covering all our code paths isn't actually sufficient to make sure that all old files can be read by newer Julia, but at least the situation is not too bad.

coveralls · 2015-04-25T17:38:01Z

Changes Unknown when pulling d596a3d on tuplefix into * on master*.

It seems that tuples are now allowed as type parameters (and have been for some time; 0.3 works) so this can be simplified. This also fixes a test failure in readremote.jl on 0.4

coveralls · 2015-04-25T19:30:23Z

Changes Unknown when pulling 64b9c30 on tuplefix into * on master*.

coveralls · 2015-04-25T19:30:24Z

Changes Unknown when pulling 64b9c30 on tuplefix into * on master*.

simonster · 2015-04-25T19:38:25Z

I can't figure out what's going on with the release build on OS X, which is intermittently failing with a completely empty log (??), but I suspect that's not related to the package at all. Other than that I think this is ready to merge.

timholy · 2015-04-25T20:43:41Z

src/jld_types.jl

@@ -631,6 +659,9 @@ function jldatatype(parent::JldFile, dtype::HDF5Datatype)
        if T == UnsupportedType
            warn("type $typename not present in workspace; reconstructing")
            T = reconstruct_type(parent, dtype, typename)
+        elseif T == Type
+            # https://github.com/JuliaLang/julia/issues/10998


Likewise here?

Probably not. I'll check.

timholy · 2015-04-25T20:47:52Z

It all looks great. Thanks to both of you! Merge at will.

Converts between Tuple{} and () types depending on Julia version. Also fixes #218

On 0.4, we can't differentiate between these and tuples of types that were saved by earlier versions of Julia. In earlier versions of Julia, such tuples were not bits typed so they would not have been usable anyway. We can revisit this when we're ready to change the file format (perhaps as part of #204/#218)

This is a bit of a hack, but necessary to avoid collision

coveralls · 2015-04-25T21:15:03Z

Changes Unknown when pulling 12479c5 on tuplefix into * on master*.

Fix tuple types for 0.4

rened · 2015-04-27T07:44:26Z

Thanks to you two for tackling the hard parts!

timholy · 2015-04-27T09:21:32Z

I didn't do anything, you two deserve the thanks!

Rene Donner added 5 commits April 20, 2015 09:11

adapt method definitions for tuple types

e811f1e

solve .names issue

5c115bd

remove unnecessary @compat

fbdc631

use fieldnames instead of .names

76a0ce0

correct FixedArray params

91d2c41

rened mentioned this pull request Apr 24, 2015

WIP: adapt method definitions for tuple types #228

Closed

simonster reviewed Apr 24, 2015
View reviewed changes

simonster changed the title ~~WIP: fix tupe types for 0.4~~ WIP: fix tuple types for 0.4 Apr 24, 2015

simonster force-pushed the tuplefix branch from 3f6fba8 to e7e0594 Compare April 25, 2015 05:02

simonster force-pushed the tuplefix branch from 3e346bb to ad00d79 Compare April 25, 2015 05:47

timholy mentioned this pull request Apr 25, 2015

.types field changed from tuple to SimpleVector JuliaLang/julia#10999

Closed

simonster added 3 commits April 25, 2015 15:14

Remove DimSize wrapper

b183249

It seems that tuples are now allowed as type parameters (and have been for some time; 0.3 works) so this can be simplified. This also fixes a test failure in readremote.jl on 0.4

Remove workarounds for #176 (since it's fixed)

b92cf62

Add missing compat to JLD00

5c2652b

simonster force-pushed the tuplefix branch from d596a3d to 64b9c30 Compare April 25, 2015 19:18

simonster changed the title ~~WIP: fix tuple types for 0.4~~ Fix tuple types for 0.4 Apr 25, 2015

timholy reviewed Apr 25, 2015
View reviewed changes

Update JLD for JuliaLang/julia#10380

ac464ac

simonster force-pushed the tuplefix branch from 64b9c30 to 12479c5 Compare April 25, 2015 21:01

simonster added 3 commits April 25, 2015 17:01

Improve type translation for cross-version compatibility

6cf6f3b

Converts between Tuple{} and () types depending on Julia version. Also fixes #218

Run JLDArchives test after workspace()

12479c5

This is a bit of a hack, but necessary to avoid collision

simonster added a commit that referenced this pull request Apr 25, 2015

Merge pull request #232 from timholy/tuplefix

74d7f05

Fix tuple types for 0.4

simonster merged commit 74d7f05 into master Apr 25, 2015

simonster deleted the tuplefix branch April 25, 2015 21:37

simonster mentioned this pull request Apr 25, 2015

HDF5 failed to load on Julia 0.4 dev #231

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tuple types for 0.4 #232

Fix tuple types for 0.4 #232

rened commented Apr 24, 2015

fundamental commented Apr 24, 2015

rened commented Apr 24, 2015

simonster Apr 24, 2015

simonster commented Apr 24, 2015

simonster Apr 24, 2015

timholy Apr 25, 2015

simonster Apr 25, 2015

simonster commented Apr 24, 2015

simonster commented Apr 24, 2015

sebastiang commented Apr 24, 2015

simonster commented Apr 24, 2015

sebastiang commented Apr 24, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

timholy commented Apr 25, 2015

simonster commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

simonster commented Apr 25, 2015

timholy Apr 25, 2015

simonster Apr 25, 2015

timholy commented Apr 25, 2015

coveralls commented Apr 25, 2015

rened commented Apr 27, 2015

timholy commented Apr 27, 2015

Fix tuple types for 0.4 #232

Fix tuple types for 0.4 #232

Conversation

rened commented Apr 24, 2015

fundamental commented Apr 24, 2015

rened commented Apr 24, 2015

simonster Apr 24, 2015

Choose a reason for hiding this comment

simonster commented Apr 24, 2015

simonster Apr 24, 2015

Choose a reason for hiding this comment

timholy Apr 25, 2015

Choose a reason for hiding this comment

simonster Apr 25, 2015

Choose a reason for hiding this comment

simonster commented Apr 24, 2015

simonster commented Apr 24, 2015

sebastiang commented Apr 24, 2015

simonster commented Apr 24, 2015

sebastiang commented Apr 24, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

timholy commented Apr 25, 2015

simonster commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

coveralls commented Apr 25, 2015

simonster commented Apr 25, 2015

timholy Apr 25, 2015

Choose a reason for hiding this comment

simonster Apr 25, 2015

Choose a reason for hiding this comment

timholy commented Apr 25, 2015

coveralls commented Apr 25, 2015

rened commented Apr 27, 2015

timholy commented Apr 27, 2015