DrChainsaw / ONNXNaiveNASflux.jl

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

Consider adding ONNXmutable to the General registry #46

Closed vtjeng closed 3 years ago

vtjeng commented 3 years ago

I'm planning to use ONNXmutable to import onnx files for use in my own package (https://github.com/vtjeng/MIPVerify.jl), and distribution would be a lot easier if there's a version of ONNXmutable that my release could be pinned to. Please consider adding ONNXmutable to the General registry!


I'm not sure whether this is relevant, but it looks like this might not be straightforward because the current released version of ONNX and the most recent commit on ONNXmutable required different versions of ProtoBuf?

# with ONNXmutable installed

(@v1.5) pkg> add ONNX
  Resolving package versions...
ERROR: Unsatisfiable requirements detected for package ProtoBuf [3349acd9]:
 ProtoBuf [3349acd9] log:
 ├─possible versions are: [0.6.1, 0.7.0, 0.8.0, 0.9.0-0.9.1, 0.10.0] or uninstalled
 ├─restricted to versions 0.9.0 by ONNXmutable [cf2a63a0], leaving only versions 0.9.0
 │ └─ONNXmutable [cf2a63a0] log:
 │   ├─possible versions are: 0.1.0 or uninstalled
 │   └─ONNXmutable [cf2a63a0] is fixed to version 0.1.0
 └─restricted by compatibility requirements with ONNX [d0dd6a25] to versions: [0.6.1, 0.7.0, 0.8.0] — no versions left
   └─ONNX [d0dd6a25] log:
     ├─possible versions are: 0.1.0-0.1.1 or uninstalled
     └─restricted to versions * by an explicit requirement, leaving only versions 0.1.0-0.1.1
DrChainsaw commented 3 years ago

Please consider adding ONNXmutable to the General registry!

Haha, yes I sure have and I think I will do it, especially now when a non-zero number of users have asked for it :)

What has been holding me back is mainly #14 as I'm not fully happy with forcing dependencies to NaiveNASflux (which has heavy dependencies like JuMP) to people who just want to export their models. Since there is some movement in the ML ecosystem community to create a better and more flexible ONNX system I might scrap the idea of #14 and just work towards extracting functionality out of ONNXmutable into libraries which it can depend on.

This leaves me with the second problem that the name ONNXmutable kinda sucks. Once it is in the general registry its no longer possible to change the name, only create a new package. The two main reasons why I haven't gotten around to change it is 1) I can't think of another name and 2) I'm not sure exactly how to do so without screwing over people who use the package.

Anyways, I'll try to resolve this soon, possibly just accepting that the name is bad and make a release. Thanks again for showing interest and good luck with MIPVerify. It looks very cool!

the current released version of ONNX and the most recent commit on ONNXmutable required different versions of ProtoBuf

Nothing to worry about. This package does not depend on ONNX which is kinda outdated and scheduled for a revamp with a completely different scope

vtjeng commented 3 years ago

I think it would definitely be helpful to split ONNXmutable into libraries with different functionality, but it would take some thought about how to design things properly. As I understand it, your library includes:

Perhaps the three could be organized in something like ONNXBase, ONNXFlux, ONNXMutableBase?

I think JuMP may provide a good model for this, with a base of MathProgBase / MathOptInterface, a higher level interface of JuMP, and individual interfaces for each solver (Cbc, GLPK, Gurobi...). Here are the rough parallels I see

This leaves me with the second problem that the name ONNXmutable kinda sucks.

I agree with this because it doesn't really summarize all the things the package does ("oh yeah, download ONNXMutable to import your graph!") but splitting the packages might help with that.

DrChainsaw commented 3 years ago

I wrote a long essay about how I think one could structure things here: https://github.com/FluxML/ML-Coordination-Tracker/issues/10

Things holding me back is basically just getting the energy/motivation to do so (contrary to what it might seem like, I don't love working with ONNX :) ) as well as creating a single person ONNX ecosystem that noone but me will understand how to use. I think JuMP was designed by a somewhat large group of people with stakes in the ecosystem and despite what everyone says about design by comitte it does have the big advantage that when the stuff materializes there is a large enough group of people who knows how the parts shall fit together.

If I/we go for something like in the link, I think this package can stay as is, just that most functionality is handled by dependencies. Mutable is there because the thing that makes CompGraph unique is the JuMP backed mutation capabilities, meaning it is very easy to modify the graph. Maybe I should just do what all the internet entrepreneurs do and give it a short sassy nonsensical name, maybe Buckler (yeah, I would not succeed as an internet entrepreneur).

vtjeng commented 3 years ago

I wrote a long essay about how I think one could structure things here: FluxML/ML-Coordination-Tracker#10

Thanks, this was very helpful.

I think JuMP was designed by a somewhat large group of people with stakes in the ecosystem and despite what everyone says about design by comitte it does have the big advantage that when the stuff materializes there is a large enough group of people who knows how the parts shall fit together.

This is a good point in favor of keeping things as is foir ONNXmutable

DrChainsaw commented 3 years ago

FYI, I have now added ONNXmutable to the general registry under the name ONNXNaiveNASflux. That name might be even worse than ONNXmutable from a marketing pov, but it at least makes it clear that it is a bit of a niche package once/if more canonical ONNX packages materialize.