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/05/23 05:22:42 UTC
[GitHub] [tvm] Tantalus13A98B5F opened a new pull request #8113: [AutoTVM] Re-enable `ref_input`
Tantalus13A98B5F opened a new pull request #8113:
URL: https://github.com/apache/tvm/pull/8113
This PR aims to implement for https://discuss.tvm.apache.org/t/autotvm-interface-for-fixed-input-data/9893.
@areusch
--
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] Tantalus13A98B5F commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
Tantalus13A98B5F commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r638399365
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -575,18 +596,22 @@ def run_through_rpc(
f_preproc=f_prepare,
)
- try:
- random_fill = remote.get_function("tvm.contrib.random.random_fill")
- except AttributeError:
- raise AttributeError(
- "Please make sure USE_RANDOM is ON in the config.cmake " "on the remote devices"
- )
- args = [nd.empty(x[0], x[1], dev) for x in build_result.arg_info]
- if "scatter" not in measure_input.task.name:
- # the index tensor of scatter op cannot be randomly initialized
- for arg in args:
- random_fill(arg)
- dev.sync()
+ if ref_input:
+ args = [nd.array(x, device=dev) for x in ref_input]
+ else:
+ try:
+ random_fill = remote.get_function("tvm.contrib.random.random_fill")
+ except AttributeError:
+ raise AttributeError(
+ "Please make sure USE_RANDOM is ON in the config.cmake "
+ "on the remote devices"
+ )
+ args = [nd.empty(x[0], x[1], dev) for x in build_result.arg_info]
+ if "scatter" not in measure_input.task.name:
+ # the index tensor of scatter op cannot be randomly initialized
+ for arg in args:
+ random_fill(arg)
+ dev.sync()
Review comment:
I guess you mean if `dev.sync` is in its right position w.r.t. the `scatter` branch. In fact it seems not for me, but I'm not sure; just inherited this piece of code from the previous commit. Confirmation needed.
--
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] areusch commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r645848464
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -242,6 +243,22 @@ def __init__(
self.executor = LocalExecutor(timeout=timeout * (self.n_parallel + 1))
+ @property
+ def ref_input(self):
+ """Fixed input for tuning special operators."""
+ return self._ref_input if hasattr(self, "_ref_input") else None
Review comment:
that makes sense to me @Tantalus13A98B5F ! i think you could also potentially put it on `MeasureInput` if you wanted to encapsulate the interface 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] [tvm] Tantalus13A98B5F commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
Tantalus13A98B5F commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r666665220
##########
File path: python/tvm/autotvm/measure/measure.py
##########
@@ -184,6 +184,8 @@ def measure_option(builder, runner):
Specify how to build programs
runner: Runner
Specify how to run programs
+ ref_input: list of np.arrays
Review comment:
yes, finding it strange when `measure_option` is shared for multiple tasks. I'm fine with reverting the `measure_option` change. the runner property will be enough for now. maybe someday we can seperate general opts and operator-spec opts for the tuner.
--
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] Tantalus13A98B5F commented on pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
Tantalus13A98B5F commented on pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#issuecomment-871925509
- Synced with up-to-date `main`.
- `measure_option` interface added. `MeasureInput` is created in tuners from tasks, maybe too impactful for now.
- Unittest added.
--
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 merged pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged pull request #8113:
URL: https://github.com/apache/tvm/pull/8113
--
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] Tantalus13A98B5F commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
Tantalus13A98B5F commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r638396696
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -242,6 +243,22 @@ def __init__(
self.executor = LocalExecutor(timeout=timeout * (self.n_parallel + 1))
+ @property
+ def ref_input(self):
+ """Fixed input for tuning special operators."""
+ return self._ref_input if hasattr(self, "_ref_input") else None
Review comment:
The aim for using `property` is to raise warning whenever setting the attribute. In prior versions, there is no entry for `ref_input` in the ctor arg, and it will be set after the object is initialized. Thus IMO it is reasonable to do the warnings and check we want in a separate setter function rather than the ctor. But yes, we can add an optional entry in the ctor, and the `if` expression can definitely be resolved.
--
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] areusch commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r638925859
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -575,18 +596,22 @@ def run_through_rpc(
f_preproc=f_prepare,
)
- try:
- random_fill = remote.get_function("tvm.contrib.random.random_fill")
- except AttributeError:
- raise AttributeError(
- "Please make sure USE_RANDOM is ON in the config.cmake " "on the remote devices"
- )
- args = [nd.empty(x[0], x[1], dev) for x in build_result.arg_info]
- if "scatter" not in measure_input.task.name:
- # the index tensor of scatter op cannot be randomly initialized
- for arg in args:
- random_fill(arg)
- dev.sync()
+ if ref_input:
+ args = [nd.array(x, device=dev) for x in ref_input]
+ else:
+ try:
+ random_fill = remote.get_function("tvm.contrib.random.random_fill")
+ except AttributeError:
+ raise AttributeError(
+ "Please make sure USE_RANDOM is ON in the config.cmake "
+ "on the remote devices"
+ )
+ args = [nd.empty(x[0], x[1], dev) for x in build_result.arg_info]
+ if "scatter" not in measure_input.task.name:
+ # the index tensor of scatter op cannot be randomly initialized
+ for arg in args:
+ random_fill(arg)
+ dev.sync()
Review comment:
actually i was referring to the outer `if ref_input:`. but i think i'm retracting my ask here, because i believe the point of `sync` is to ensure `random_fill` has been materialized into memory.
--
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] areusch commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r665548173
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -235,13 +236,30 @@ def __init__(
self.number = number
self.repeat = repeat
self.min_repeat_ms = min_repeat_ms
+ self._ref_input = None
self.enable_cpu_cache_flush = enable_cpu_cache_flush
self.cooldown_interval = cooldown_interval
self.module_loader = module_loader
self.executor = LocalExecutor(timeout=timeout * (self.n_parallel + 1))
+ @property
+ def ref_input(self):
+ """Fixed input for tuning special operators."""
Review comment:
could you qualify "special" (explain it is for operators that cannot handle random input)
##########
File path: python/tvm/autotvm/measure/measure.py
##########
@@ -184,6 +184,8 @@ def measure_option(builder, runner):
Specify how to build programs
runner: Runner
Specify how to run programs
+ ref_input: list of np.arrays
Review comment:
apologies but on looking at this change in code now, i think it may be a bit too fine-grained to add directly to measure_option. it seems like we should either encapsulate the per-tuning-run options in an e.g. `operator_opts` kwarg to measure_option or just keep them in RPCRunner for now. it seems like if we continue with this pattern, we'd want to do an `operator_opts` reorganization at some future point anyhow, so i'd rather not start down that path for one option right now. i suggest we stick with the RPCRunner property for now then, and improve the interface as we add more options. what do you think?
##########
File path: tests/python/unittest/test_autotvm_measure.py
##########
@@ -60,8 +62,24 @@ def test_task_tuner_without_measurement_spawn():
p.join()
+def test_task_runner_with_ref_input():
+ """test runner ref_input without measurement"""
+ refinp = [np.random.rand(128, 128) for i in range(3)]
+ measure_option = autotvm.measure_option(builder="local", runner="local", ref_input=refinp)
+
+ class DummyExecutor(measure.executor.Executor):
+ def submit(self, func, *args, **kwargs):
+ sig = Signature.from_callable(measure.measure_methods.run_through_rpc)
+ assert sig.bind(*args, **kwargs).arguments["ref_input"] == refinp
Review comment:
can you set a variable here in the outer `test_task_runner_with_ref_input` (e.g. `ran_dummy_executor = True`) and then assert on that in the test to ensure that this function and assert ran?
--
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 #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#issuecomment-881845061
Thanks @Tantalus13A98B5F @areusch! It is merged 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Tantalus13A98B5F commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
Tantalus13A98B5F commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r639415485
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -242,6 +243,22 @@ def __init__(
self.executor = LocalExecutor(timeout=timeout * (self.n_parallel + 1))
+ @property
+ def ref_input(self):
+ """Fixed input for tuning special operators."""
+ return self._ref_input if hasattr(self, "_ref_input") else None
Review comment:
just come across a new idea. it would be more reasonable to set `ref_input` on `measure_option` rather than `RPCRunner`; what's more, it can be implemented by just forwarding `ref_input` to the runner object, and we can still make the warning when setting the runner object. `Runner(ref_input=*)` seems somewhat strange to me.
--
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] areusch commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r638924271
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -242,6 +243,22 @@ def __init__(
self.executor = LocalExecutor(timeout=timeout * (self.n_parallel + 1))
+ @property
+ def ref_input(self):
+ """Fixed input for tuning special operators."""
+ return self._ref_input if hasattr(self, "_ref_input") else None
Review comment:
ah that's right. i think it would be a better design to keep with the pattern of ctor arg and not modify attributes of the runner after construction. however, if the value needs to change during tuning, I'm fine with the setter--just would suggest to drop `hasattr` here and instead, in ctor, include: `self._ref_input = None`
--
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] areusch commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r638260969
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -242,6 +243,22 @@ def __init__(
self.executor = LocalExecutor(timeout=timeout * (self.n_parallel + 1))
+ @property
+ def ref_input(self):
+ """Fixed input for tuning special operators."""
+ return self._ref_input if hasattr(self, "_ref_input") else None
Review comment:
seems as though we could just set `self._ref_input` to `None` in `__init__` if it is not set. also, why this style of setting, rather than taking as an `__init__` arg?
##########
File path: python/tvm/autotvm/measure/measure_methods.py
##########
@@ -575,18 +596,22 @@ def run_through_rpc(
f_preproc=f_prepare,
)
- try:
- random_fill = remote.get_function("tvm.contrib.random.random_fill")
- except AttributeError:
- raise AttributeError(
- "Please make sure USE_RANDOM is ON in the config.cmake " "on the remote devices"
- )
- args = [nd.empty(x[0], x[1], dev) for x in build_result.arg_info]
- if "scatter" not in measure_input.task.name:
- # the index tensor of scatter op cannot be randomly initialized
- for arg in args:
- random_fill(arg)
- dev.sync()
+ if ref_input:
+ args = [nd.array(x, device=dev) for x in ref_input]
+ else:
+ try:
+ random_fill = remote.get_function("tvm.contrib.random.random_fill")
+ except AttributeError:
+ raise AttributeError(
+ "Please make sure USE_RANDOM is ON in the config.cmake "
+ "on the remote devices"
+ )
+ args = [nd.empty(x[0], x[1], dev) for x in build_result.arg_info]
+ if "scatter" not in measure_input.task.name:
+ # the index tensor of scatter op cannot be randomly initialized
+ for arg in args:
+ random_fill(arg)
+ dev.sync()
Review comment:
does this need to run for both if branches?
--
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] Tantalus13A98B5F commented on a change in pull request #8113: [AutoTVM] Re-enable `ref_input`
Posted by GitBox <gi...@apache.org>.
Tantalus13A98B5F commented on a change in pull request #8113:
URL: https://github.com/apache/tvm/pull/8113#discussion_r666665220
##########
File path: python/tvm/autotvm/measure/measure.py
##########
@@ -184,6 +184,8 @@ def measure_option(builder, runner):
Specify how to build programs
runner: Runner
Specify how to run programs
+ ref_input: list of np.arrays
Review comment:
yes, finding it strange when `measure_option` is shared for multiple tasks. I'm fine with reverting the `measure_option` change, and maybe we can set an argument on `Tuner.tune` instead? That should be always operator-specific.
--
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