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 2021/08/19 00:15:18 UTC
[GitHub] [tvm] shingjan opened a new pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
shingjan opened a new pull request #8789:
URL: https://github.com/apache/tvm/pull/8789
This PR intends to add `initializer` as well as `initargs` to `popenpool` api.
@tqchen @vinx13 @junrushao1994
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r691691524
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -153,6 +160,11 @@ def _start(self):
self._reader = os.fdopen(main_read, "rb")
self._writer = os.fdopen(main_write, "wb")
+ # init
+ if self._initializer is not None:
+ self.send(self._initializer, self._initargs)
+ self.recv()
Review comment:
I am not sure about the logic here. Looks like the workflow is:
* `PopenWorker.send` is the entry point, which calls `PopenWorker._start` if the worker is not available
* with this logic, the `PopenWorker._start` method will call back to `PopenWorker.send` to send the initializer to the worker process.
I didn't see any problem with the workflow itself, but the cyclic call is a bit indirect. Would you mind if we move these 4 lines into the `if self._proc is None` check?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-901518963
BTW, if we have a file containing user-registered functions, can we send this to the worker process so that the worker is aware of these registration? I suppose it is as simple as sending an "import XXX"
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692379797
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -86,8 +86,12 @@ class PopenWorker:
API to interact with a separate process via Popen.
"""
- def __init__(self):
+ def __init__(self, initializer=None, initargs=()):
Review comment:
done
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -276,14 +288,19 @@ class PopenPoolExecutor:
behavior of multiprocessing.pool().
"""
- def __init__(self, max_workers=None, timeout=None):
+ def __init__(self, max_workers=None, timeout=None, initializer=None, initargs=()):
Review comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692370486
##########
File path: tests/python/contrib/test_popen_pool.py
##########
@@ -66,6 +66,19 @@ def test_popen_pool_executor():
assert val.value == idx
+def test_popen_initializer():
+ initargs = [1, 2, 3]
+ pool = PopenPoolExecutor(
Review comment:
removed
##########
File path: python/tvm/testing/popen_pool.py
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, missing-function-docstring
+"""Common functions for popen_pool test cases"""
+
+a = 0
Review comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-902329460
Oh looks like I need @vinx13's approval to merge this PR, but he is now on vacation 😄
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692605491
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -269,21 +285,32 @@ class PopenPoolExecutor:
timeout : float
Timeout value for each function submit.
- Note
Review comment:
Keep this line
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on a change in pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r691688309
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -276,14 +288,19 @@ class PopenPoolExecutor:
behavior of multiprocessing.pool().
"""
- def __init__(self, max_workers=None, timeout=None):
+ def __init__(self, max_workers=None, timeout=None, initializer=None, initargs=()):
Review comment:
please add docs for the args
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -128,6 +132,9 @@ def _start(self):
if self._proc is not None:
return
+ if self._initializer is not None:
Review comment:
remove this
##########
File path: python/tvm/testing/popen_pool.py
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, missing-function-docstring
+"""Common functions for popen_pool test cases"""
+
+a = 0
Review comment:
consider using meaningful variable names
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -86,8 +86,12 @@ class PopenWorker:
API to interact with a separate process via Popen.
"""
- def __init__(self):
+ def __init__(self, initializer=None, initargs=()):
Review comment:
please add docs for the args
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-901514283
Thanks @shingjan for the contribution! This work definitely makes Popen more complete, in terms of customizable initialization of workers :-)
BTW, please document the usage of this argument, so that users could understand what is going on under the hood
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692370647
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -128,6 +132,9 @@ def _start(self):
if self._proc is not None:
return
+ if self._initializer is not None:
Review comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-902331067
you can dismiss the reviews as long as the comments are addressed
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692369734
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -153,6 +160,11 @@ def _start(self):
self._reader = os.fdopen(main_read, "rb")
self._writer = os.fdopen(main_write, "wb")
+ # init
+ if self._initializer is not None:
+ self.send(self._initializer, self._initargs)
+ self.recv()
Review comment:
done
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-902327025
Overall it looks pretty good now :-) Please fix the lint and let's get it merged ASAP
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-902325825
@junrushao1994 I added three new test cases for `ffi`, including calling python function via ffi, calling cpp function via ffi and calling python function from cpp function via ffi in `popenworker`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on a change in pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r691688941
##########
File path: python/tvm/testing/popen_pool.py
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=invalid-name, missing-function-docstring
+"""Common functions for popen_pool test cases"""
+
+a = 0
Review comment:
consider using meaningful variable names, e.g `TEST_GLOBAL_STATE`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-901523306
> BTW, if we have a file containing user-registered functions, can we send this to the worker process so that the worker is aware of these registration? I suppose it is as simple as sending an "import XXX"
In this case the registry map need to be sent. Sending an 'import' may also work, it requires the user file that calls register_func be available in PYTHONPATH
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 merged pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
vinx13 merged pull request #8789:
URL: https://github.com/apache/tvm/pull/8789
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on a change in pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r691691114
##########
File path: tests/python/contrib/test_popen_pool.py
##########
@@ -66,6 +66,19 @@ def test_popen_pool_executor():
assert val.value == idx
+def test_popen_initializer():
+ initargs = [1, 2, 3]
+ pool = PopenPoolExecutor(
Review comment:
`pool` is not used
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692565036
##########
File path: tutorials/autotvm/tune_conv2d_cuda.py
##########
@@ -204,7 +204,7 @@ def conv2d_no_batching(N, H, W, CO, CI, KH, KW, stride, padding):
# see many error reports. As long as you can see non-zero GFLOPS, it is okay.
tuner = autotvm.tuner.XGBTuner(task)
tuner.tune(
- n_trial=20,
+ n_trial=100,
Review comment:
it is removed. I was changing this for benchmarking. Slipping through my finger lol
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r692563697
##########
File path: tutorials/autotvm/tune_conv2d_cuda.py
##########
@@ -204,7 +204,7 @@ def conv2d_no_batching(N, H, W, CO, CI, KH, KW, stride, padding):
# see many error reports. As long as you can see non-zero GFLOPS, it is okay.
tuner = autotvm.tuner.XGBTuner(task)
tuner.tune(
- n_trial=20,
+ n_trial=100,
Review comment:
probably we don't need this :-)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on a change in pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on a change in pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#discussion_r693094300
##########
File path: python/tvm/contrib/popen_pool.py
##########
@@ -269,21 +285,32 @@ class PopenPoolExecutor:
timeout : float
Timeout value for each function submit.
- Note
Review comment:
Thanks @junrushao1994 for adding this line and @vinx13 for the comments!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] shingjan commented on pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
shingjan commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-902330521
I think Github used to let contributors revoke `request for changes`. But maybe that is not the case for public repository like TVM.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] vinx13 commented on pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
vinx13 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-901515613
Thanks @shingjan , this will unblock refactor of using PopenPool in autotvm
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8789: [CONTRIB] Add initializer to popenpool as well as test cases
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-901528736
@vinx13 this snippet does work, but i am not sure if it is the best way to do:
```python
import importlib.util
spec = importlib.util.spec_from_file_location("mymodule", "/home/jrshao/Projects/tvm-tensorir-dev/tests/python/tir/util.py")
foo = importlib.util.module_from_spec(spec)
spec.loader.exec_module(foo)
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8789: [CONTRIB] Allow customized initializer in PopenPool
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8789:
URL: https://github.com/apache/tvm/pull/8789#issuecomment-901654426
@shingjan please address the comments and add a testcase for registering extra methods with `tvm._ffi.register_func` and `tvm._ffi.get_global_func`. Let's get it merged ASAP :-)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org