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/01 18:24:20 UTC

[GitHub] [incubator-mxnet] samskalicky opened a new pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

samskalicky opened a new pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812


   ## Description ##
   Fixes #19811
   
   


----------------------------------------------------------------
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] leezu commented on pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771692779






----------------------------------------------------------------
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] leezu merged pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
leezu merged pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812


   


----------------------------------------------------------------
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 edited a comment on pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771234624


   > Thank you! Could you please add a test? It looks like this was completely untested?
   
   Looking at the flow, it seems to only happen when calling `optimize_for` twice. After each call the `backend` and `backend_opts` are cleared:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/python/mxnet/gluon/block.py#L1398-L1401
   
   And then the "clearing" sets the `backend_opts` to None. So on the 2nd call, if the user does not provide any options it would run into this scenario (where backend_opts is None). In the lib_subgraph extensions example we call optimize_for once to partition, and then a 2nd time to run a graph pass:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/example/extensions/lib_subgraph/test_subgraph.py#L65-L78
   
   But in the unittests, we only use the `default` subgraph backends:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/tests/python/unittest/test_subgraph_op.py#L267
   
   So theres no way to duplicate exactly what is being done in the example. But, we do build the `lib_subgraph` library in the CMakeLists.txt:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/CMakeLists.txt#L814
   
   So we could add a test in unittests to use this library. But I think we decided not to do this in the past since people were running the unittests on pip wheels that didnt have the libraries packaged into the wheel. Thoughts?


----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771234624


   > Thank you! Could you please add a test? It looks like this was completely untested?
   
   Looking at the flow, it seems to only happen when calling `optimize_for` twice. After each call the `backend` and `backend_opts` are cleared:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/python/mxnet/gluon/block.py#L1398-L1401
   
   And then the "clearing" sets the `backend_opts` to None. So on the 2nd call, if the user does not provide any options it would run into this scenario. In the lib_subgraph extensions example we call optimize_for once to partition, and then a 2nd time to run a graph pass:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/example/extensions/lib_subgraph/test_subgraph.py#L65-L78
   
   But in the unittests, we only use the `default` subgraph backends:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/tests/python/unittest/test_subgraph_op.py#L267
   
   So theres no way to duplicate exactly what is being done in the example. But, we do build the `lib_subgraph` library in the CMakeLists.txt:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/CMakeLists.txt#L814
   
   So we could add a test in unittests to use this library. But I think we decided not to do this in the past since people were running the unittests on pip wheels that didnt have the libraries packaged into the wheel. Thoughts?


----------------------------------------------------------------
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 edited a comment on pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771234624


   > Thank you! Could you please add a test? It looks like this was completely untested?
   
   Looking at the flow, it seems to only happen when calling `optimize_for` twice. After each call the `backend` and `backend_opts` are cleared:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/python/mxnet/gluon/block.py#L1398-L1401
   
   And then the "clearing" sets the `backend_opts` to None. So on the 2nd call, if the user does not provide any options it would run into this scenario (where backend_opts is None). In the lib_subgraph extensions example we call optimize_for once to partition, and then a 2nd time to run a graph pass:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/example/extensions/lib_subgraph/test_subgraph.py#L65-L78
   
   But in the unittests, we only use the `default` subgraph backends:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/tests/python/unittest/test_subgraph_op.py#L267
   
   So theres no way to duplicate exactly what is being done in the example. But, we do build the `lib_subgraph` library in the CMakeLists.txt:
   https://github.com/apache/incubator-mxnet/blob/49edbfc82cb0ce3b823051e1acc26388bc79686c/CMakeLists.txt#L814
   
   So we could add a test in unittests to use this library. But I think we decided not to do this in the past since people were running the unittests on pip wheels that didnt have the libraries packaged into the wheel. Thoughts?


----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771234624






----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#discussion_r568199667



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1052,7 +1052,7 @@ def __init__(self):
         self._callback = None
         self._monitor_all = False
         self._backend = None
-        self._backend_opts = {}
+        self._backend_opts = dict()

Review comment:
       will change to literal




----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#discussion_r568136470



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1052,7 +1052,7 @@ def __init__(self):
         self._callback = None
         self._monitor_all = False
         self._backend = None
-        self._backend_opts = {}
+        self._backend_opts = dict()

Review comment:
       I always had the impression that initialization with literal empty dict is preferred.




----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771060497


   Hey @samskalicky , 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**: [centos-cpu, unix-cpu, clang, website, unix-gpu, edge, windows-cpu, sanity, miscellaneous, centos-gpu, windows-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] leezu commented on pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-771692779


   > But I think we decided not to do this in the past since people were running the unittests on pip wheels that didnt have the libraries packaged into the wheel. Thoughts?
   
   The CI does not use wheels for most pipelines AFAIK. You can just add a separate test folder and execute it manually in one of the CI pipelines?


----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-772091561


   Jenkins CI successfully triggered : [windows-gpu, windows-cpu, 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 pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
samskalicky commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-772119122


   > The CI does not use wheels for most pipelines AFAIK. You can just add a separate test folder and execute it manually in one of the CI pipelines?
   
   I found a file `test_extensions.py` in tests/python/unittest and added the relevant code from the example so that it will be tested in the CI. Thanks for the discussion!


----------------------------------------------------------------
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] leezu commented on a change in pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#discussion_r568171085



##########
File path: python/mxnet/gluon/block.py
##########
@@ -1052,7 +1052,7 @@ def __init__(self):
         self._callback = None
         self._monitor_all = False
         self._backend = None
-        self._backend_opts = {}
+        self._backend_opts = dict()

Review comment:
       The only exception may be function / method arguments




----------------------------------------------------------------
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] leezu commented on pull request #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-772091517


   @mxnet-bot run ci [unix-cpu, windows-cpu, windows-gpu]


----------------------------------------------------------------
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 #19812: [2.0.alpha] fixed backend_opts to be empty dictionary instead of None

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #19812:
URL: https://github.com/apache/incubator-mxnet/pull/19812#issuecomment-772091561


   Jenkins CI successfully triggered : [windows-gpu, windows-cpu, 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