You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/04/04 08:16:10 UTC
[GitHub] [incubator-tvm] ANSHUMAN87 opened a new pull request #5236:
[WIP][TVM][.NET] Introduce TVM.NET project
ANSHUMAN87 opened a new pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236
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.
NOTE: This PR is under active development, please do not consider for review. Once it is ready for review will welcome all reviewers. However all proposals or feature request will be welcomed.
Thanks you very much!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5236: [WIP][TVM][.NET]
Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-609046500
please mark the PR as draft before it is ready for review to reduce the burden on the CI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5236: [WIP][TVM][.NET]
Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-609046534
https://github.blog/2019-02-14-introducing-draft-pull-requests/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen commented on issue #5236: [WIP][TVM][.NET]
Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-609062776
both sounds fine
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] ANSHUMAN87 commented on issue #5236:
[WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-609062283
> 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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] tqchen edited a comment on issue #5236:
[WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-609046500
Thanks for the proposal, please mark the PR as draft before it is ready for review to reduce the burden on the CI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] ANSHUMAN87 commented on issue #5236:
[WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-616160551
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/tvm/pull/5236#issuecomment-819065129
Given the stale state, let us close this for now. Thanks @ANSHUMAN87 @jroesch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on issue #5236: [WIP][TVM][.NET]
Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-615954623
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-tvm] ANSHUMAN87 edited a comment on issue #5236:
[WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 edited a comment on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-615921445
> 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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tvm] tqchen closed pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen closed pull request #5236:
URL: https://github.com/apache/tvm/pull/5236
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on issue #5236:
[WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on issue #5236: [WIP][TVM][.NET] Introduce TVM.NET project
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-615921445
> 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)?
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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [tvm] tqchen commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5236:
URL: https://github.com/apache/tvm/pull/5236#issuecomment-819065129
Given the stale state, let us close this for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #5236:
URL: https://github.com/apache/tvm/pull/5236#issuecomment-819064012
@tqchen @ANSHUMAN87 I am doing triage, what is the status on this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-622821231
@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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-631500869
@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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626223676
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-631521753
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626186249
> @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 !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-631521753
Please be patient. Careful code reveiws takes time and in many time sthey are as valuable, if not more valuable than the contribution itself. We certainly need more eyes for runtime related contributions. 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626228705
@tqchen : Thanks a lot! I got your points clearly now!
Will reorganize code as suggested and improvise further!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626208924
@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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626193368
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(on top of the mainline) and improve it before mainlining it back. Please also feel free to refer to other language bindings for reference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626193368
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626193368
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 mroe careful works to meet the standard of our current language binding. 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 codebase. 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.
Given the current state, you could try release the package elsewhere and improve it before mainlining it back. Please also feel free to refer to other language bindings for reference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626193368
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 codebase. 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.
Given the current state, you could try release the package elsewhere and improve it before mainlining it back. Please also feel free to refer to other language bindings for reference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] ANSHUMAN87 commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-629841268
@tqchen : Your review comments are handled now. Please check. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-631521753
Please be patient. Careful code reveiws takes time and in many time sthey are as valuable, if not more valuable than the contribution itself. 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-631521753
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 runtime related contributions. 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626223676
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen edited a comment on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626193368
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #5236: [WIP][TVM][.NET] Introduce TVM.NET project
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #5236:
URL: https://github.com/apache/incubator-tvm/pull/5236#issuecomment-626193368
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 we want. 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 mroe careful works to meet the standard of our current language binding. 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 codebase. 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.
Given the current state, you could try release the package elsewhere and improve it before mainlining it back. Please also feel free to refer to other language bindings for reference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org