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

Consider adding ONNXmutable to the General registry #46

Closed
vtjeng opened this issue Mar 14, 2021 · 5 comments
Closed

Consider adding ONNXmutable to the General registry #46

vtjeng opened this issue Mar 14, 2021 · 5 comments

Comments

@vtjeng
Copy link

vtjeng commented Mar 14, 2021

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

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

vtjeng commented Mar 15, 2021

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:

  • Deserialization/serialization between onnx <--> a general graph format (something close to Fresh start FluxML/ONNX.jl#46)
  • Conversion between a general graph format <--> a graph format that wraps Flux operations
  • Helper functions that allow mutation on top of the general graph format

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

I wrote a long essay about how I think one could structure things here: FluxML/FluxML-Community-Call-Minutes#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
Copy link
Author

vtjeng commented Mar 16, 2021

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

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.

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