Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break out primitives to separate packages #55

Open
DrChainsaw opened this issue Aug 15, 2021 · 3 comments
Open

Break out primitives to separate packages #55

DrChainsaw opened this issue Aug 15, 2021 · 3 comments

Comments

@DrChainsaw
Copy link
Owner

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 FluxML/ONNX.jl#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
Copy link
Owner Author

@dfdx

@ToucheSir
Copy link

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
Copy link
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants