You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2021/02/05 17:58:29 UTC
[GitHub] [incubator-mxnet] szha opened a new pull request #19850: [PIP] update manifest to include lib_api.cc
szha opened a new pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850
## Description ##
update manifest to include lib_api.cc
## Checklist ##
### Essentials ###
- [x] PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
- [x] Changes are complete (i.e. I finished coding on this PR)
- [x] All changes have test coverage
- [x] Code is well-documented
### Changes ###
- [x] update manifest to include lib_api.cc
## Comments ##
- If this change is a backward incompatible change, why must this change be made.
- Interesting edge cases to note here
----------------------------------------------------------------
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-mxnet] mxnet-bot commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774190753
Hey @szha , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [sanity, windows-gpu, miscellaneous, clang, website, unix-cpu, centos-cpu, edge, unix-gpu, windows-cpu, centos-gpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
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-mxnet] samskalicky commented on a change in pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#discussion_r571171367
##########
File path: tools/pip/MANIFEST.in
##########
@@ -29,4 +29,5 @@ recursive-include mxnet *_LICENSE
recursive-include mxnet *.h
recursive-include mxnet *.hpp
recursive-include mxnet *.cuh
+recursive-include mxnet *.cpp
Review comment:
cool, were you able to package the wheel and unzip it and confirm the lib_api.cc was inside?
----------------------------------------------------------------
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-mxnet] samskalicky commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774727125
@mxnet-bot run ci [unix-cpu]
----------------------------------------------------------------
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-mxnet] szha commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774207271
@samskalicky I think both need to be kept for now. The needs for packaging is quite different from setting up the package 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
[GitHub] [incubator-mxnet] samskalicky commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774202902
Which setup.py do we use (or do we want to use) on master moving forward:
https://github.com/apache/incubator-mxnet/blob/master/tools/pip/setup.py
https://github.com/apache/incubator-mxnet/blob/master/python/setup.py
Can we remove the other 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-mxnet] samskalicky commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774307038
everything else looks good to me, can you confirm packaging the wheel now includes the lib_api.cc?
----------------------------------------------------------------
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-mxnet] samskalicky commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774208780
> @samskalicky I think both need to be kept for now. The needs for packaging is quite different from setting up the package locally.
makes sense, so the one in tools/pip/setup.py is the one we use for building wheels, everywhere (dist.mxnet.io, releases, etc)?
----------------------------------------------------------------
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-mxnet] szha commented on a change in pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#discussion_r571165011
##########
File path: tools/pip/MANIFEST.in
##########
@@ -29,4 +29,5 @@ recursive-include mxnet *_LICENSE
recursive-include mxnet *.h
recursive-include mxnet *.hpp
recursive-include mxnet *.cuh
+recursive-include mxnet *.cpp
Review comment:
actually no, let me include cc as well.
----------------------------------------------------------------
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-mxnet] szha commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774311118
And now we are good.
----------------------------------------------------------------
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-mxnet] mxnet-bot commented on pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#issuecomment-774727150
Jenkins CI successfully triggered : [unix-cpu]
----------------------------------------------------------------
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-mxnet] samskalicky commented on a change in pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#discussion_r571160254
##########
File path: tools/pip/MANIFEST.in
##########
@@ -29,4 +29,5 @@ recursive-include mxnet *_LICENSE
recursive-include mxnet *.h
recursive-include mxnet *.hpp
recursive-include mxnet *.cuh
+recursive-include mxnet *.cpp
Review comment:
does *.cpp include *.cc files?
https://github.com/apache/incubator-mxnet/blob/master/src/lib_api.cc
----------------------------------------------------------------
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-mxnet] szha commented on a change in pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850#discussion_r571273515
##########
File path: tools/pip/setup.py
##########
@@ -103,6 +103,7 @@
os.path.join(CURRENT_DIR, 'mxnet/include/nnvm'))
# copy cc file for mxnet extensions
+os.mkdir(os.path.join(CURRENT_DIR, 'mxnet/src'))
shutil.copy(os.path.join(CURRENT_DIR, 'mxnet-build/src/lib_api.cc'),
os.path.join(CURRENT_DIR, 'mxnet/src'))
Review comment:
without creating src folder, the semantics of this is to create a regular file called `src` with the content of lib_api.cc...
----------------------------------------------------------------
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-mxnet] szha merged pull request #19850: [PIP] update manifest to include lib_api.cc
Posted by GitBox <gi...@apache.org>.
szha merged pull request #19850:
URL: https://github.com/apache/incubator-mxnet/pull/19850
----------------------------------------------------------------
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