Protobuf v1

[lambe]

• Upgrades repo to use v1 of Protobuf.jl
• Updates read.jl and write.jl to handle new convention
• Fixes unit tests

[DrChainsaw]

Changes look good all in all. Thank you very much!

Please delete all the temporary dirs (i.e. pb_scratch, onnx and pbjl_scratch) in the root dir.

[DrChainsaw]

Could we make this less hideous looking by having something like

const TensorProto_DataType = var"TensorProto.DataType"

in BaseOnnx.jl?

[lambe]

Sure, I took your suggestion in the latest commits.

[DrChainsaw]

Just checking if you need some assistance here. Seems like the constructor signature of protostructs have changed in ProtoBuf version 1.0 which causes pretty much every test case to fail. Hopefully it is just a few changes needed in https://github.com/DrChainsaw/ONNXNaiveNASflux.jl/blob/master/src/baseonnx/write.jl to make things pass.

[lambe]

Yes, I could definitely use a hand. I was experimenting with making changes in test/validate.jl but I'm a little stuck.

My big worry is that, because the structs are immutable by default, a whole lot of test rewriting is needed. You're a better judge of this code, though.

[DrChainsaw]

My big worry is that, because the structs are immutable by default, a whole lot of test rewriting is needed.

Yeah, this could be painful, or not. Not sure yet. I got a bit scared initially since I thought it would hit all over serialize.jl, but it should be quite ok since the nodes of the graph is still a vector and most of the mutation in there just pushes to that vector.

Lets take it one thing at the time. Latest commit I pushed fixed the constructors for a couple of protostructs used in the first two testsets in readwrite.jl. I suggest you take a look at the changes and try to make the last testset in there work.

The main two things I needed to change was:

1. Usage of enumx instead primitive integers for things like TensorProto_DataType.FLOAT. Since the constructors still want the primitive one needs to extract the enum ordinal which according to the brief readme for enumx seems to be done by calling Integer, e.g. Integer(TensorProto_DataType.INT64). Just to be somewhat forward compatible I added a new function called enumordinal so it would be easier to swap in the future.
2. A type called OneOf which indicates a type which has one of several possible fields, instead of having a Dict{Symbol, Any} where constructors now expect a OneOf instead of just providing the keyword argument for the type you want to set. I can't understand why they didn't just allow one to use the kwarg and made OneOf in the constructor in ProtoBuf.jl.

There are some migration tips here (although I did not find them very helpful for this work): https://juliaio.github.io/ProtoBuf.jl/dev/#Migrating-from-earlier-versions-of-ProtoBuf.jl

[lambe]

Thanks for your help. I got most of the last testset working - see latest commit - but the Dict is returning an error

  Test threw exception
  Expression: (((pairs(attrs) |> collect) .|> AttributeProto) .|> serdeser) |> Dict == attrs
  ArgumentError: Dict(kv): kv needs to be an iterator of tuples or pairs

This should be covered by the last line of read.jl

Base.Dict(pa::AbstractVector{AttributeProto}) = Dict(attribute(p) for p in pa)

but the output of the serdeser function doesn't seem to be recognized as a vector of AttributeProtos.

[DrChainsaw]

This should be covered by the last line of read.jl

Hipshot guess here is that this is because the AttributeProto now have type parameters so the method definition needs to change to

Base.Dict(pa::AbstractVector{<:AttributeProto}) = Dict(attribute(p) for p in pa)

Note the extra <:.

[lambe]

Base.Dict(pa::AbstractVector{<:AttributeProto}) = Dict(attribute(p) for p in pa)

Note the extra <:.

Direct hit! readwrite.jl tests are all green now :)

[DrChainsaw]

I guess you got occupied with other stuff. Let me know if you intend to resume this work.

[lambe]

@DrChainsaw yeah, to be honest I got stuck and moved on to other things. This is well above my expertise in Julia.

[codecov]

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.07%. Comparing base (9a194f7) to head (48ec798).

Files	Patch %	Lines
src/baseonnx/write.jl	91.66%	1 Missing ⚠️
src/serialize/serialize.jl	87.50%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #85   +/-   ##
=======================================
  Coverage   96.06%   96.07%           
=======================================
  Files          15       15           
  Lines        1042     1044    +2     
=======================================
+ Hits         1001     1003    +2     
  Misses         41       41           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DrChainsaw]

Thanks for the help @lambe. Turns out you were about 80% there. Sorry for the long delay.