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 move the repo somewhere else #20

Open
Hzfengsy opened this issue Nov 30, 2021 · 4 comments
Open

Consider move the repo somewhere else #20

Hzfengsy opened this issue Nov 30, 2021 · 4 comments

Comments

@Hzfengsy
Copy link
Contributor

Synr is a tvm-related project so far, which helps TVMScript works well. It does a great job to make AST parser stable across different python versions. However, it is not easy to update, since:

  1. It is a dependent repo. There are so many steps if we need to add new features or fix bugs: make a PR; then wait for merge and new release; update TVM install script; add TVM feature and ut.
  2. The repo under OctoML will block some of the contributors (due to some company policy) and we must wait for the guys in OctoML to release new releases, which makes it harder to fix bugs.

There are two possible proposals here:

  1. Move it to tlc-pack: tlc-pack is the convention where we maintain tvm-related dependency. More people can manage it there.
  2. Merging into tvm is the best way to make things maintainable. So, we don't need to install a specific version of synr for dev on a specific branch

cc @tqchen @jroesch @junrushao1994

@junrushao
Copy link

junrushao commented Nov 30, 2021

Either options work for me.

Personally I hit lots of pain switching synr version in-between branches, and frontend development (e.g. @shingjan’s recent PR) needs to update both sides and keep doing minor releases, which is less convenient. Moreover, contributors to this repo should be honored in tvm too, so merging into TVM might be a better option IMO.

@shingjan
Copy link
Member

Will vote for integrating synr into TVM as moving it into tlcpack would still leave the process of checking synr changes into TVM cumbersome.

@tkonolige
Copy link
Contributor

Hi @Hzfengsy, there are definitely some challenges with maintaining synr as a separate repo, but I think they are outweighed by the benefits. To address your points specifically:

  1. Yes, there are a couple more steps involved in updating synr, but the benefit we get is that synr does not have to go through the TVM CI (which takes a long time). I think we've also done a good job of merging PRs faster than what you would see in the TVM repo.
  2. Have we had any contributors have this issue? The benefit here is that it is easier to run CI for the synr repo.

Regarding counter arguments to moving to specific repos:

  1. There doesn't seem to be any benefit to moving to tic-pack besides maybe consistency. And then we'll have to setup CI and we won't be able to use the (freely provided) OctoML CI.
  2. There are two options here: 1. we integrate synr in tree 2. we make synr a submodule. For 1. we now have to go through TVM CI for all changes, which takes a long time. It also prevents other projects from using synr, which was one of the original goals of the project. For 2. now users need to add another path to their PYTHOPATH and need to ensure they always keep the submodule up to date. This would be better handled by fixing dependency handling in TVM.

For your issue @junrushao1994, the only solution would be to put synr in-tree for TVM, which then hits the issues with slow CI and the inability for other projects to use synr.

The main reason why we started synr as a separate project was so that other projects could use synr for their ast parsing needs.

@shingjan
Copy link
Member

shingjan commented Dec 1, 2021

@tkonolige Thanks for the clarification. My points:

  1. It is a good thing that as a separate repo, synr could have its own CI. But almost every change we bring to synr will end up on TVM, meaning the change we bring to synr/TVM will need to go through TVM CI anyway. Not to mention the fact that there is cost to run CI on its own and updating the version of synr in TVM.
  2. Maintenance cost. Right now every time we update the version of synr, one TVM developer will need to fetch the latest TVM and manually fetch the latest synr from pip to make it work as synr is referenced roughly at three different places in the repo. This process could be confusing for first timers. Same issue will exist if we make synr a submodule, likely running git submodule update all the time.
  3. We couldn't spot any change runs fine on synr's CI but not on TVM's CI now. There is no way to do that until we update the version of synr. And at that time we still need to decide if we want to roll back or roll out a new version, in which neither of them seems ideal.

I believe that right now synr is an active project we will be working on extensively. As a major dependency of TVM we could integrate it into the TVM tree so we can enjoy the benefit while making TVM the best DL compiler in the field. And if the development of synr is stabilizing and many other projects find this piece of work useful then we can separate this project again.

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

4 participants