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

Fresh start #46

Merged
merged 3 commits into from
Mar 24, 2021
Merged

Fresh start #46

merged 3 commits into from
Mar 24, 2021

Conversation

dfdx
Copy link
Collaborator

@dfdx dfdx commented Mar 2, 2021

As discussed earlier, I'm posting this PR as a fresh start for the ONNX package. It includes utils for reading and writing ONNX's ProtoBuf definitions from https://github.com/DrChainsaw/ONNXmutable.jl/tree/master/src/baseonnx. All corresponding tests pass, but there's still a couple of things update before merging:

  • current repo contains the paper describing the original design of ONNX.jl; the paper is outdated, but I guess removing it may have negative academic influence on the authors - I need advice what to do here
  • README is outdated, I will replace it with one describing the current status of the package
  • CI files (Travis) are outdated, will replace it with GitHub Actions\
  • anything else?

@ayush-1506
Copy link
Member

ayush-1506 commented Mar 3, 2021

@dfdx Thanks for picking this up, really appreciate it. If you're revamping the structure of the package, the paper perhaps loses its relevance since it outlines the earlier design choice. I'd say its better to create a new folder (something like artifacts or assets or archives) and push the paper directory there. This way there would be a record of this paper and a new paper can also be added in the future, if needed.
Thanks again!

@dfdx dfdx changed the title [WIP] Fresh start Fresh start Mar 3, 2021
@dfdx
Copy link
Collaborator Author

dfdx commented Mar 3, 2021

@ayush-1506 Thanks, this approach looks reasonable!

I've finalized the changes described above, so the PR is ready for review.

@ayush-1506
Copy link
Member

@dfdx Thanks again, looks like a lot of changes. I'll try to review the changes in the coming days. Also tagging @philtomson here.
Phil, would be great if you could also review this if you have some spare time at your disposal. Thanks!

@philtomson
Copy link
Collaborator

I feel like I'm pretty far out of the loop here. I'm going to defer to @ayush-1506.

@dfdx
Copy link
Collaborator Author

dfdx commented Mar 9, 2021

Let me reiterate on what this PR is so that it doesn't look like much of work. The changes boil down to:

  • removing nearly everything from the repo
  • adding low-level utils for reading and writing ONNX files (src/read.jl, src/write.jl + tests in test/readwrite.jl)
  • updating dependencies, bumping package version, moving from Travis to Github Actions and other minor changes

All the code belongs to @DrChainsaw who kindly allowed me to create this PR, the background for this (quite huge) change can be found in the discussion on the FluxML tracker.

I hope it simplifies your reviews a little bit :)

@dfdx
Copy link
Collaborator Author

dfdx commented Mar 21, 2021

Bump. Please let me know if there's anything I can do to help with the review (e.g. more context, unaddressed questions, concerns, etc.).

Copy link
Member

@ayush-1506 ayush-1506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay. Looks good, just a few changes and clarifications would be helpful.

* [old version of this README](https://github.com/FluxML/ONNX.jl/blob/b7c3d0b48036947257e439c31e00430b0a94690a/README.md)
* [RFC for the new implementation](https://github.com/FluxML/ML-Coordination-Tracker/discussions/24)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if you could add new usage instructions in the readme (even if there's no fixed api at the moment) and a few lines about how this works under the hood.

@@ -0,0 +1,72 @@
@testset "Read and write" begin
import ONNX

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfdx I'm wondering if these tests are enough. Don't we also need operator level tests? Basically tests for our mappings of https://github.com/onnx/onnx/blob/master/docs/Operators.md

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is more level thing so far. The details can be found on the discussion, but the main points are:

  • this package in its current state is essentially unmaintained
  • there are several competing ONNX libraries out their used in different contexts
  • it would be great to have a single implementation - preferably here - that works not only for Flux, but also for other projects (e.g. Avalon.jl which I maintain)
  • we don't have agreement on the high-level API yet (e.g. ONNX operators)
  • but there's a set of low-level utilities for reading and writing ONNX's ProtoBuf definitions
  • the plan is to settle these low-level utilities first and based on it move our discussion of operators further

As you can see from the code, there are only convenient methods for reading and writing ONNX entities without the need to dive into all the subtleties of the format specification. This is also the reason why there are no proper usage examples - it is simply not designed for the end users (yet).

I realize this is a huge change, but since this package doesn't work with the latest Flux version anyway, I think it's fair to step back and redesign everything almost from scratch. Yet I'll be happy to see alternative approaches to the problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

@ayush-1506
Copy link
Member

@dfdx Thanks for the effort, I'm merging this for now. Would be great if you add a short plan for the next steps somewhere to keep track of these developments.

@ayush-1506 ayush-1506 merged commit cec4859 into FluxML:master Mar 24, 2021
@dfdx dfdx deleted the fresh-start branch March 24, 2021 05:52
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

Successfully merging this pull request may close these issues.

3 participants