Fix all but one 1.7 issues #103

mauro3 · 2021-09-30T12:15:34Z

This builds on PR #99 (i.e. it is branched from there) and fixes some more Julia 1.7 issues. It does not fix the issue with reconstructing anonymous functions. I think this is what PR #102 is about, however, that one did not get Jameson's blessing and I'm not sure what he meant in his comment #102 (comment).

If resolving that issue takes a long time, it could be a possibility to merge this PR and get the last 5% sorted in another PR (instead of having a non-functioning BSON in Julia 1.7)

This PR seems to work for what I need on Julia 1.7. So even if it is not merged, it maybe of use to others...

mauro3 · 2021-09-30T12:17:02Z

src/extensions.jl

-structdata(x) = isprimitive(typeof(x)) ? reinterpret_(UInt8, [x]) :
-    Any[getfield(x, f) for f in fieldnames(typeof(x))]
+structdata(x) = isprimitivetype(typeof(x)) ? reinterpret_(UInt8, [x]) :
+    Any[getfield(x,f) for f in fieldnames(typeof(x)) if isdefined(x, f)]


The if isdefined(x, f) was necessary to avoid errors. I don't actually know what it does... So, if this leads to a bug, it would be good to have a test-case which shows this.

What are the errors? We'd need all the fields of a struct to reconstruct it iiuc

One of the fields was undefined thus access to it threw an error:

Primitive Types: Error During Test at /home/mauro/julia/dot-julia-dev/BSON/test/runtests.jl:41 Test threw exception Expression: roundtrip_equal(Tuple) UndefRefError: access to undefined reference Stacktrace: [1] structdata @ ~/julia/dot-julia-dev/BSON/src/extensions.jl:96 [inlined]

This is from the@test roundtrip_equal(Tuple) test. Everything else works.

I just tried @test roundtrip_equal(Tuple{Int,Float64}) and that works as well.

Just to clarify, the roundtrip_equal(Tuple) test passes with master + 1.6 but errors with 1.7?

Yes, because one of the fields for Vararg{T} is undefined on construction. See my comment.

DhairyaLGandhi · 2021-09-30T13:00:28Z

This looks great! We will need some test cases that check whether the if statement can cause any trouble. I can't think of anything off the bat, but perhaps something along the lines of a struct that holds #undef in its fields?

DhairyaLGandhi · 2021-09-30T13:01:32Z

Do we know the missing piece in reconstructing anonymous functions?

mauro3 · 2021-09-30T15:41:13Z

Ok, I had a look at it. The bug was already present in Julia 1.6 but the tests did not reconstruct a undef field. In Julia 1.7 something changed with Vararg such that Tuple now contains an undef. I now added a test to directly test this (which fails on Julia 1.6 and BSON-master).

So, it would probably be ok to merge this now (after reviews) and do the anonymous function support in another PR.

(I used this debug function if anyone wants to follow along:

function structdata(x)
    if !isprimitivetype(typeof(x))
        println("")
        @show typeof(x), isprimitivetype(typeof(x))
        @show x
        @show [(f, isdefined(x,f)) for f in fieldnames(typeof(x))]
    end
    isprimitivetype(typeof(x)) ? reinterpret_(UInt8, [x]) :
    Any[getfield(x,f) for f in fieldnames(typeof(x)) if isdefined(x, f)]
end

)

darsnack · 2021-10-09T14:28:53Z

This seems to be the same changes as #102. I think it should approach the undefined field issue the same way by having a restricted path for Vararg{T} instead of adding the isdefined check for all structs.

DhairyaLGandhi · 2021-10-09T14:38:15Z

It's hard to guarantee that undefs wouldn't be present in code in the wild, and that would need to be handled for every type we encounter. Plus this gives a better chance at forward add backward compatibility with existing bson dumps since it's reusing Base functions.

darsnack · 2021-10-09T14:52:58Z

Yes, but if you see the tests add in #102, undefined references in structs are already handled by the current code base. It's specifically the N type parameter being undefined in TypeofVararg that's the isolated case.

#102 uses the same Base functions as here to address the problems here. The non-Base functions were me trying to fix something else that already used non-Base functions (i.e. I didn't add non-Base functions to the code).

Not saying we should merge #102 instead or anything. I'm only repeating the review to #102 here because a) it was in response to your advice on #102 so I figured the same applies here, and b) it minimizes the surface area of the code changes.

mauro3 · 2021-10-09T22:16:51Z

test/runtests.jl

@@ -39,6 +44,9 @@ end
  @test roundtrip_equal("b")
  @test roundtrip_equal([1,"b"])
  @test roundtrip_equal(Tuple)
+  @test roundtrip_equal(Tuple{Int, Float64})
+  @test roundtrip_equal(Vararg{Any})
+  @test roundtrip_equal(Bar()) # test reconstructing #undef field


Note that this test failed on 1.6 already.

There's a similar test for NoInit() in #102 that passes on 1.6 without the isdefined check for all structs.

It would be good to copy the "Undefined References" test set from #102 to track the issue.

mauro3 · 2021-10-09T22:19:14Z

Ah, yes, I didn't see that you handled that already in #102. I guess I only saw all the ccall stuff and blanked ;-)

Note that the test case @test roundtrip_equal(Bar()), which I added in this PR, fails on current master and with Julia 1.6.

darsnack · 2021-10-09T22:28:46Z

No problem 😉. I think this PR is a better refactor of the the same fixes anyways.

The only suggestions I have are copying over the tests added in #102. And I'm wondering why the anonymous function tests are only pre-1.7 now? Those tests appear to be passing on nightly for #102.

The ccall stuff was just me copying over old ccall code and updating for 1.7. But based on Jameson's comments, I think the newstruct_raw code needs to be updated to not use ccall. His suggestion was to just eval a struct definition.

mauro3 · 2021-10-09T22:54:22Z

Thanks, I added your tests.

The anonymous function tests error on Julia-1.7-rc1 with

Anonymous Functions: Error During Test at /home/mauro/julia/dot-julia-dev/BSON/test/runtests.jl:104                                                                                             
  Got exception outside of a @test                                                              
  type DataType has no field ninitialized

darsnack · 2021-10-09T23:05:15Z

Okay, got it, so the ccall stuff in #102 was to fix that. I would mark the test set as broken instead of only running pre-1.7. We can merge this, and I can take a look at #102 to fix the issue Jameson's way. Then I'll mark the tests as not broken again.

mauro3 · 2021-10-09T23:09:04Z

It is marked as "throws". As the first line of the test errors, then I only test that first throw.
https://github.com/JuliaIO/BSON.jl/pull/103/files#diff-3b9314a6f9f2d7eec1d0ef69fa76cfabafdbe6d0df923768f9ec32f27a249c63R117

darsnack · 2021-10-09T23:11:09Z

Circling back on the undefined fields issues.

Can we try restricting the isdefined check to only TypeofVararg like #102 and see if the tests are passing here?

It's possible that we should have that check for every struct, but we want to think about how we reconstruct the struct in that situation (i.e. Dhairya's concern). So I'd rather take the change slowly and separately from getting 1.7 working.

mauro3 · 2021-10-09T23:25:38Z

If I revert line https://github.com/JuliaIO/BSON.jl/pull/103/files#diff-241e225da364e0237227e298e47eca392b7b3830b02c0fc6ccc095107cffdc57R97, i.e. remove the isdefined check, then, on Julia 1.6 (where Vararg does not error because of undef), I get the following:

Undefined References: Error During Test at /home/mauro/julia/dot-julia-dev/BSON/test/runtests.jl:62                                                                                             
  Test threw exception                                                                                                                                                                          
  Expression: roundtrip_equal(Bar())

So, no, the tests as they are in this PR do not pass without the isdefined.

Well, my take is: this PR seems to be an improvement (as far as tested in the test-suite) both for 1.7-compatibility and undef-handling. So, it seems a bit pointless to make this PR less universal by only doing the Vararg special case. You can still take it slowly afterwards..

DhairyaLGandhi · 2021-10-09T23:33:36Z

Cool, this seems good to go to me. @mauro3 wdyt?

Other than that, we probably need to add some additional handling around DataType explicitly.

darsnack · 2021-10-09T23:35:34Z

Okay I guess the ccall stuff is addressing multiple other issues. I'm fine merging this now.

mauro3 · 2021-10-09T23:42:56Z

Yes, this is ready to be merged as far as I'm concerned. (Probably would be good to tag as well sometime soon)

daviehh and others added 2 commits June 3, 2021 17:45

julia 1.7: mutable field fix

23c65b3

Mostly fixed the 1.7 issues except for annonymous function test

445548a

mauro3 commented Sep 30, 2021

View reviewed changes

mauro3 changed the title ~~Fix some 1.7 issues~~ Fix all but one 1.7 issues Sep 30, 2021

mauro3 mentioned this pull request Sep 30, 2021

julia 1.7: mutable field fix #99

Merged

mauro3 commented Oct 9, 2021

View reviewed changes

Added tests to reconstruct an undef fields

4fd3ae9

mauro3 force-pushed the m3/fix17 branch from 6f959e6 to 4fd3ae9 Compare October 9, 2021 22:51

darsnack approved these changes Oct 9, 2021

View reviewed changes

DhairyaLGandhi approved these changes Oct 9, 2021

View reviewed changes

DhairyaLGandhi merged commit 3b4a2ce into JuliaIO:master Oct 9, 2021

mauro3 deleted the m3/fix17 branch October 9, 2021 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix all but one 1.7 issues #103

Fix all but one 1.7 issues #103

mauro3 commented Sep 30, 2021 •

edited

Loading

mauro3 Sep 30, 2021

DhairyaLGandhi Sep 30, 2021

mauro3 Sep 30, 2021

DhairyaLGandhi Sep 30, 2021 •

edited

Loading

darsnack Oct 9, 2021

DhairyaLGandhi commented Sep 30, 2021

DhairyaLGandhi commented Sep 30, 2021

mauro3 commented Sep 30, 2021

darsnack commented Oct 9, 2021

DhairyaLGandhi commented Oct 9, 2021

darsnack commented Oct 9, 2021

mauro3 Oct 9, 2021

darsnack Oct 9, 2021

mauro3 commented Oct 9, 2021

darsnack commented Oct 9, 2021 •

edited

Loading

mauro3 commented Oct 9, 2021

darsnack commented Oct 9, 2021

mauro3 commented Oct 9, 2021

darsnack commented Oct 9, 2021

mauro3 commented Oct 9, 2021

DhairyaLGandhi commented Oct 9, 2021 •

edited

Loading

darsnack commented Oct 9, 2021

mauro3 commented Oct 9, 2021

Fix all but one 1.7 issues #103

Fix all but one 1.7 issues #103

Conversation

mauro3 commented Sep 30, 2021 • edited Loading

mauro3 Sep 30, 2021

Choose a reason for hiding this comment

DhairyaLGandhi Sep 30, 2021

Choose a reason for hiding this comment

mauro3 Sep 30, 2021

Choose a reason for hiding this comment

DhairyaLGandhi Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

darsnack Oct 9, 2021

Choose a reason for hiding this comment

DhairyaLGandhi commented Sep 30, 2021

DhairyaLGandhi commented Sep 30, 2021

mauro3 commented Sep 30, 2021

darsnack commented Oct 9, 2021

DhairyaLGandhi commented Oct 9, 2021

darsnack commented Oct 9, 2021

mauro3 Oct 9, 2021

Choose a reason for hiding this comment

darsnack Oct 9, 2021

Choose a reason for hiding this comment

mauro3 commented Oct 9, 2021

darsnack commented Oct 9, 2021 • edited Loading

mauro3 commented Oct 9, 2021

darsnack commented Oct 9, 2021

mauro3 commented Oct 9, 2021

darsnack commented Oct 9, 2021

mauro3 commented Oct 9, 2021

DhairyaLGandhi commented Oct 9, 2021 • edited Loading

darsnack commented Oct 9, 2021

mauro3 commented Oct 9, 2021

mauro3 commented Sep 30, 2021 •

edited

Loading

DhairyaLGandhi Sep 30, 2021 •

edited

Loading

darsnack commented Oct 9, 2021 •

edited

Loading

DhairyaLGandhi commented Oct 9, 2021 •

edited

Loading