Create a repo for separating ONNX optimizer out of ONNX repo

[linkerzhang]

ONNX optimizer has not been actively maintained and updated by ONNX community, however, it's still being used by some partners. To have ONNX repo focus on ONNX spec itself and also not break any potential dependency on ONNX optimizer, we'd create a repo (optimizer) under ONNX org to develop and maintain it. @daquexian @fumihwh will help to make the move initially.

[askhade]

Before we create a repo
@linkerzhang , @daquexian , @fumihwh : Are you also planning to fix all the issues in the optimizers?
There are some fundamental issues with optimizers today... They need to be fixed for IR versions 4 and above. See PR for context: onnx/onnx#2247

@daquexian @fumihwh : Can you explain what dependencies do you have on these optimizers? Most of these optimizers are already covered by converters, are there any which are not being covered today?
Can you evaluate the python based optimizers : https://github.com/microsoft/onnxconverter-common/blob/master/onnxconverter_common/optimizer.py
If these can cover your scenarios then its best if you use these...

[daquexian]

@linkerzhang , @daquexian , @fumihwh : Are you also planning to fix all the issues in the optimizers?
There are some fundamental issues with optimizers today... They need to be fixed for IR versions 4 and above. See PR for context: onnx/onnx#2247

@askhade I'm planning to fix existing issues in optimizers.

@daquexian @fumihwh : Can you explain what dependencies do you have on these optimizers? Most of these optimizers are already covered by converters, are there any which are not being covered today?
Can you evaluate the python based optimizers : https://github.com/microsoft/onnxconverter-common/blob/master/onnxconverter_common/optimizer.py
If these can cover your scenarios then its best if you use these...

In my opinion, there are many mobile inference frameworks (like ncnn, tengine) having an onnx converter written in C++, most of which don't have a complicated optimization procedure and have to rely on a standalone onnx optimizer (offline optimization), or an onnx optimizer C++ library (embedded in converter). This python based optimizer in onnxconverter_common repo cannot cover these scenarios.

What's more, a C++ optimizer enables some interesting applications. convertmodel.com, a website that runs onnx optimizer without any installation and compilation, uses webassembly compiled from the current C++ optimizer, while the same thing will be much harder to achieve if onnx optimizer is written in python.

[linkerzhang]

@prasanthpul @harryskim @jimspohrer @joohoon @szha please Steering Committee members also show your opinions if any.

The high-level action plan of this move should be,

1. Create optimizer repo under onnx org.
2. Move optimizer lib from onnx repo to optimizer repo WITHOUT any API changes.
3. In 1.8 ONNX release, call out to public that optimizer lib in ONNX repo will be removed and replaced with the one in optimizer repo.
4. Remove optimizer codes in ONNX repo (release it in 1.9).
5. Maintain/Develop optimizer in optimizer repo separately.

The step 3/4 maybe merged in one release (1.8) if folks all agree.

[fumihwh]

@askhade
I am using C++ API to implement custom optimizers in my project.
As onnx is an open source project, we could not just REMOVE some features and make folks migrate by paying high cost.
Agree with @linkerzhang , we could do something like that.

[askhade]

@fumihwh : The basis of this move was that optimizers are broken and they do not work on IRv4 onwards. If folks have a hard dependency on this then yes we can move optimizer to a different repo instead of deleting it. I am curious how this is working for you today.

@linkerzhang : Perhaps we should take this opportunity to move not just the optimizers but version-converter and c++ IR out of ONNX and move all these to onnx-tools repo instead of creating a special repo for optimizers.

[spandantiwari]

I agree with the idea of creating a separate tools repo and moving optimizers and version-converters to that repo. It is quite possible (we need to look) that IRv4 failures is because of issues in both the graph IR classes and the client-side code (e.g. optimizer). In that case if we want to support version converter to work with ir_version > 3, we may have to make updates to the code (IR classes).

If we decide to create a separate repo for onnx tools, another point to consider is whether we want to move the ONNX checker tool to that repo as well.

[askhade]

I prefer keeping onnx-checker in the onnx repo.

onnx checker is central to ONNX. Every model which is converted to onnx format is checked for validity using this checker tool. We need to keep it updated and valid. This will (most likely) not be true with tools repo. Ideally all tools under this repo should be updated and checked for correctness against latest onnx opset but there is no such promise being made right now. Tomorrow if we figure that this is not maintained or for whatever reason it is not required then we can deprecate this repo. Adding checker to this repo will prohibit us from doing this.

[linkerzhang]

C++ IR is being used by optimizer so that it will be moved. Version converters should not be moved if we want to advocate folks to keep updating it (for conversion between versions). Yes, ONNX checker should be in ONNX repo. Any other stuff in our mind should be moved out? if not, then I'll suggest to use "optimizer" as the new repo name, since a "tools" repo is too general and it will make the repo a dumping place.

[fumihwh]

@fumihwh : The basis of this move was that optimizers are broken and they do not work on IRv4 onwards. If folks have a hard dependency on this then yes we can move optimizer to a different repo instead of deleting it. I am curious how this is working for you today.

@askhade Optimizers work at least with ONNX 1.6.0 (IR v6). They are not broken, just buggy. I am using them and even C++ API to implement custom optimizers now. I think I am not only one who use optimizer C++ API.

[askhade]

@linkerzhang : I agree with your point that if we want to advocate folks to keep updating version converter then we should keep it in ONNX but it also depends on c++ IR... that is why I was suggesting we move it to this "tools" repo along with optimizer and c++ IR.

[daquexian]

@askhade Optimizers work at least with ONNX 1.6.0 (IR v6). They are not broken, just buggy. I am using them and even C++ API to implement custom optimizers now. I think I am not only one who use optimizer C++ API.

The optimizer is not compatible with IR >= 4. It only works "by accident". onnx/onnx#2198, onnx/onnx#1718

[askhade]

@linkerzhang : Ping :)
What are your thoughts on version converter.

[linkerzhang]

@linkerzhang : Ping :)
What are your thoughts on version converter.

I'd keep it in the ONNX repo, since conversion among versions is one of frequent asks from onnx users :).

Let me know if you guys have strong preferences on moving it out.

[linkerzhang]

It is not that bad to keep (one copy) the c++ IR in onnx repo, since it's not changed much and no need updating it that frequently with ONNX update.

given my personal thought above, I'd suggest to keep using "optimizer" as the new repo name. "onnx-tools" is too general and the repo will be a dumping area with the name, I believe :). @askhade @daquexian @fumihwh @jcwchen.

[linkerzhang]

@houseroad (the main contributor of onnx optimizer) for comments please.

[houseroad]

Sorry for the late reply, quite busy these days.

I would suggest to keep this feature in the main repo before we could provide alternative solution. Especially we have active users relying on it. I didn't see any obvious benefit we can achieve from removing this optimizer from main repo. The feature is opt-in, and a few developers still actively contribute to it, such as @fumihwh and @daquexian. We just need to figure out a plan for the maintainer. Not sure if @fumihwh is interested or not to serve for sig-infra or maintain this feature. :-)

[prasanthpul]

As discussed in steering committee open meeting this week (notes: https://github.com/onnx/steering-committee/blob/master/meeting-notes/20200716.md) the plan is to move the optimizer code into a new repo. This is analogous to the separate converter repos.

The new repo will be owned by the Arch/Infra SIG and will need to be maintained. So we are looking for @fumihwh and @daquexian to confirm their support for this.

[daquexian]

Sorry for the late reply, quite busy these days.

I would suggest to keep this feature in the main repo before we could provide alternative solution. Especially we have active users relying on it. I didn't see any obvious benefit we can achieve from removing this optimizer from main repo. The feature is opt-in, and a few developers still actively contribute to it, such as @fumihwh and @daquexian. We just need to figure out a plan for the maintainer. Not sure if @fumihwh is interested or not to serve for sig-infra or maintain this feature. :-)

@fumihwh and me are both willing to support/maintain the optimizers, and I think moving it to a separate repo makes it easy to maintain and refactor.

As discussed in steering committee open meeting this week (notes: https://github.com/onnx/steering-committee/blob/master/meeting-notes/20200716.md) the plan is to move the optimizer code into a new repo. This is analogous to the separate converter repos.

The new repo will be owned by the Arch/Infra SIG and will need to be maintained. So we are looking for @fumihwh and @daquexian to confirm their support for this.

Yes, I'm willing to support it.

[prasanthpul]

Thanks @daquexian and @fumihwh. We'll setup the new "optimizer" repo.

cc: @askhade, @linkerzhang

[prasanthpul]

https://github.com/onnx/optimizer/ created. Arch/Infra SIG has access.