Skip to content

[WIP][TVM][.NET] Introduce TVM.NET project#5236

Closed
ANSHUMAN87 wants to merge 47 commits intoapache:mainfrom
ANSHUMAN87:tvm-dotnet
Closed

[WIP][TVM][.NET] Introduce TVM.NET project#5236
ANSHUMAN87 wants to merge 47 commits intoapache:mainfrom
ANSHUMAN87:tvm-dotnet

Conversation

@ANSHUMAN87
Copy link
Contributor

@ANSHUMAN87 ANSHUMAN87 commented Apr 4, 2020

This PR brings new feature into TVM:

  • Introduce C-sharp API binding to various existing TVM APIs(Currently limited to only Runtime, but future PRs will cover more.)
  • Introduce TVM.NET a framework for easy integration of TVM runtime into .NET world. This includes integration and deployment(publishing packages).
  • Sample APPs for start.

Development phase-0 is completed!

Please refer RFC

Welcome for review!
Thanks you very much!

@tqchen
Copy link
Member

tqchen commented Apr 4, 2020

Thanks for the proposal, please mark the PR as draft before it is ready for review to reduce the burden on the CI

@tqchen
Copy link
Member

tqchen commented Apr 4, 2020

https://github.blog/2019-02-14-introducing-draft-pull-requests/

@ANSHUMAN87
Copy link
Contributor Author

https://github.blog/2019-02-14-introducing-draft-pull-requests/

@tqchen : Thanks for the suggestion! I missed it, but it seems that already raised PR cant be converted to Draft PR. Please advise how to proceed, there are 2 ways i can handle it.
1:> Keep local copy until skeleton is ready, then upload all the changes.
2:> Close and reopen the PR as Draft.

Thanks!

@tqchen
Copy link
Member

tqchen commented Apr 4, 2020

both sounds fine

@ANSHUMAN87
Copy link
Contributor Author

ANSHUMAN87 commented Apr 18, 2020

both sounds fine

@tqchen : As discussed earlier, i have not uploaded any changes yet to this PR. I think it will be better to setup CI for the Dotnet workspace first. So is it okay, if i upload another PR( a sample program, i already developed)?
Which will have direct linking to TVM Runtime and a test module to check the execution on the ENV, with that we can setup CI first for a successful run, post that i can upload the working project.

Please provide your valuable opinion. Thanks!

@tqchen
Copy link
Member

tqchen commented Apr 18, 2020

We can start code reviews and getting feedbacks from the community. Since we do want to hold a high standard for new languauge bindings, when we are getting close to merge, we will look into the CI issues. In the meanwhile, please write testcases and run them locally

@ANSHUMAN87
Copy link
Contributor Author

We can start code reviews and getting feedbacks from the community. Since we do want to hold a high standard for new languauge bindings, when we are getting close to merge, we will look into the CI issues. In the meanwhile, please write testcases and run them locally

@tqchen : Thanks for your feedback! I will start uploading my changes in this PR.

@ANSHUMAN87
Copy link
Contributor Author

@tqchen : I believe phase-0 development is complete now!
Please help review! Thank you very much!

May be we can change the status of the PR now!

@ANSHUMAN87
Copy link
Contributor Author

@tqchen : I believe phase-0 development is complete now!
Please help review! Thank you very much!

May be we can change the status of the PR now!

Gentle ping @tqchen !

@tqchen
Copy link
Member

tqchen commented May 9, 2020

Language bindings are important features that we want to hold the highest standard as possible. So we do expect for longer review time, as well as more polishment.

In a nutshell, what we want is not "an implement" of the feature. But to implement the feature in the best possible way so that it can be maintained, reused and developed in the future with minimum techinical debt.

I am not expert in csharp, but by quickly look at the package, I do think it needs more careful works to reach to similar quality of our other language bindings. Here are some high level comments:

  • Structure: think about a common structure. Take a look at existing projects for example
    • e.g. mldotnet https://github.com/dotnet/machinelearning/tree/master/src
    • A possible structure for tvm could be root/donet/src/
    • I do not see any reason why we need a separate subfolder for each .cs file
    • We should not include binaries: e.g. add_one.dll into the code repo.
    • There are additional files (Class1) which seems to be created from the the getting started guide but was not used here.
  • Naming: I would try to follow other language binding naming convention as much as possible, in particular, the web, java and golang one are good references. e.g. UnmanagedRuntime
  • Layers of abstractions: there are see many indirections and manager class(from manged to unmanaged), many of them seems to be un-necessary. By convention, the FFI layer is un-managed, and all the wrappers on top are managed. Again, jvm runtime would be a good example to follow.
  • Given that a language binding needs to be maintained and reviewed separately, we should get someone who is able to collaborate and review the proposed binding. Since reviewing is as time consuming as implementing the feature, so please try to polish as much as you can.
    • Ideally, we want to have as many vets on the API design as much as possible.

You could try release the package elsewhere and improve it a bit. Please also feel free to refer to other language bindings for reference.

@ANSHUMAN87
Copy link
Contributor Author

@tqchen : Thank you very much for your enlightening response!
I will definitely work towards attaining similar code standard as per existing runtimes in TVM.

Please bear with me, if i misunderstood some of your comments. I have some queries to those as below:

  1. Folder structure: You are right it has no significance. It is just a personal choice to me in organizing the files. The reference(mldotnet) you have suggested also follow similar organization of files.
    But if you think otherwise is more suitable, we can keep all the source codes in one place or maintain Managed & Unmanaged separation similar to JVM runtime(Core & Native).

  2. Naming Convention: Do you suggest the name change from UnmanagedRuntimeWrapper --> UnmanagedRuntime as an example ?

  3. Layers of abstractions: You are right there is a very thin line boundary between Managed & Unmanaged space. Unmanaged space does not store any memory of its own in most of the cases, it is just an enabler or interface into TVM runtime. If you can provide one specific example which one you want to remove from Unmanged space, may be that might help me understand your comment more clearly.

  4. Package Release: I am not sure how to release package elsewhere and pertain to same PR. Can you please help me do it? May be some reference would be helpful.

Please help me clarify above queries.

Thank again!

@tqchen
Copy link
Member

tqchen commented May 9, 2020

By packaging the result elsewhere, i mean just create a separate repo that build on top of tvm, once the package get used and starts to mature, then send another PR to the mainline.

Please refer to the existing projects(existing language bindings or mldotnet) for reference. Here are some exaples:

If you look at other language bindings, you can find that there is no Unmanaged layer, because the user only sees the managed part of the API, and Unmanaged is part of the FFI. So that additional layer of abstraction is not necessary here, as functions can be implemented in the managed space by calling into private dll functions.

Module, NDArray and Runtime are so coupled that they do not merit a separate folder(namespace) If you look at mldotnet, there are multiple files under the same folder if they belongs to the same module.

@ANSHUMAN87
Copy link
Contributor Author

@tqchen : Thanks a lot! I got your points clearly now!
Will reorganize code as suggested and improvise further!

@ANSHUMAN87
Copy link
Contributor Author

@tqchen : Your review comments are handled now. Please check. Thanks!

@ANSHUMAN87
Copy link
Contributor Author

@tqchen : I think i have reworked on all your comments now. With the new string marshaling approach, i believe now it is more modular. Can you please take a look, and provide your valuable opinons, if any other improvements can be done. Thanks!

@tqchen
Copy link
Member

tqchen commented May 20, 2020

Please be patient. Careful code reveiws takes time and they are as valuable, if not more valuable than the contribution itself. We certainly need more eyes for a new runtime. One way to help review other PRs and get the reviewers to return in favor.

It would be useful to find another person who can co-author, review and further polish the PR, in terms of code-style and the overall implementation, since we need to keep a good maintaince of the language packages beyond a single person.

@tqchen tqchen changed the base branch from master to main October 11, 2020 18:35
@jroesch
Copy link
Member

jroesch commented Apr 13, 2021

@tqchen @ANSHUMAN87 I am doing triage, what is the status on this one?

@tqchen
Copy link
Member

tqchen commented Apr 13, 2021

Given the stale state, let us close this for now. Thanks @ANSHUMAN87 @jroesch

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