[AutoTVM] Re-enable ref_input
#8113
Conversation
There was a problem hiding this comment.
hi @Tantalus13A98B5F , thanks for opening the PR! I made a couple small comments about the approach. Could you also add a test?
| @property | ||
| def ref_input(self): | ||
| """Fixed input for tuning special operators.""" | ||
| return self._ref_input if hasattr(self, "_ref_input") else None |
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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.
|
There was a problem hiding this comment.
sorry for the long delay @Tantalus13A98B5F. i added a couple more comments and will give this PR more priority so we can merge it.
| 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 |
There was a problem hiding this 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?
|
|
||
| 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.""" |
There was a problem hiding this comment.
could you qualify "special" (explain it is for operators that cannot handle random input)
| @@ -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 | |||
There was a problem hiding this 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?
There was a problem hiding this 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.
|
Thanks @Tantalus13A98B5F @areusch! It is merged now! |
* [AutoTVM] Re-enable ref_input * add ref_input on measure_option * add ref_input unittest * fix: test reformat * [autotvm] [ref-input] refine test and description * [autotvm] [ref-input] revert arg on measure_option
* [AutoTVM] Re-enable ref_input * add ref_input on measure_option * add ref_input unittest * fix: test reformat * [autotvm] [ref-input] refine test and description * [autotvm] [ref-input] revert arg on measure_option
This PR aims to implement for https://discuss.tvm.apache.org/t/autotvm-interface-for-fixed-input-data/9893.
@areusch