search

DrChainsaw / ONNXNaiveNASflux.jl

Import/export ONNX models
MIT License
43 stars 2 forks source link

Break out primitives to separate packages #55

Open DrChainsaw opened 3 years ago

DrChainsaw commented 3 years ago

The import/export primitives for ONNX ops might be best served as a separate packages so that people can benefit from them without having to depend on this package.

Structure of such a package does probably not need to be more complicated than something like:

module OnnxPrimitivesFlux

using ONNX

onnx2dense(attributes, weight, bias) = #return a Dense 
onnx2conv(attributes, weight, bias) = #return a Conv

dense2onnx(layer::Dense) = #return a NodeProto
conv2onnx(layer::Conv) = #return a NodeProto

end

Code which can be reused is mainly in import: https://github.com/DrChainsaw/ONNXNaiveNASflux.jl/blob/master/src/deserialize/ops.jl export: https://github.com/DrChainsaw/ONNXNaiveNASflux.jl/blob/master/src/serialize/serialize.jl generic row<->col major stuff: https://github.com/DrChainsaw/ONNXNaiveNASflux.jl/blob/master/src/shapes.jl

Unfortunately, this package was made in a scratch and itch kinda way and there is probably a fair amount of stuff which needs to be surgically removed from the NaiveNASflux stuff. In particular, the code bends over backwards more than once to try to infer sizes and shapes where a 'raw' model would not need to.

It seems to make sense to put Flux primitives separate from those which doesn't require Flux. Mapping OP-types to functions is probably best done by users of primitive packages so that they can pick and chose.

I would hold off doing this at least until the discussion in https://github.com/FluxML/ONNX.jl/issues/49 has landed. Perhaps the right way to import Flux layers is to first import the ONNX model in the generic framework and the translate that model to Flux...

DrChainsaw commented 3 years ago

@dfdx

ToucheSir commented 3 years ago

As a start, can we completely ignore the *2onnx functionality? Ideally there could also be some automation of the generation of the onnx2* functions as well. Instead of using Flux layers, for example, create a struct that implements exactly the functionality of the ONNX op using only NNlib and base primitives. Then the only points needed for integration with other libraries would be row<->column major conversions and maybe Functors support.

DrChainsaw commented 3 years ago

Absolutely, its just that a handful of flux2onnx and onnx2flux happens to exist in this package.

As I wrote in the OP I think one does not need to adress this issue until FluxML/ONNX.jl#49 is more fleshed out which is meant to imply that it has higher prio imo.