[Retiarii]: Add info required by nn-meter to graph ir #3910

kaleid-liner · 2021-07-06T18:12:21Z

Tested on #3876 ShuffleNetV2

ghost · 2021-07-06T18:12:32Z

All CLA requirements met.

ultmaster · 2021-07-07T02:55:00Z

nni/retiarii/graph.py

@@ -309,7 +309,7 @@ def add_node(self, name: str, operation: Operation) -> 'Node': ...
    @overload
    def add_node(self, name: str, type_name: str, parameters: Dict[str, Any] = {}) -> 'Node': ...

-    def add_node(self, name, operation_or_type, parameters={}):
+    def add_node(self, name, operation_or_type, parameters=None):


Why change the behavior of this API?

From my knowledge, it is not a good practice to use {} as default values, here is a reference
https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/

Not sure whether there is any other special reason, but I believe it worth a refactor to change all default {} and [] to None

Why change the behavior of this API?

And this will cause parameters of the nodes to share the same dict object (the initial {}). When one of the parameters is changed, all of the others will also be changed at the same time.

This results in some strange bugs.

I'm okay with this change if you insist. But you should put a if parameters is None: parameters = {} in the body of this function.

If parameters is immutable in the function body, actually putting parameters={} in the arguments works, except that IDE will complain.

I'm okay with this change if you insist. But you should put a if parameters is None: parameters = {} in the body of this function.

Yes. I have added that to Opeartion::__init__() and Operation::__new__().

If parameters is immutable in the function body, actually putting parameters={} in the arguments works, except that IDE will complain.

But if you assign it to the attribute of the object, when next time you modify that attribute, the other objects will also be affected. (Actually this is why it incurs some strange bugs and it costs me a night to debug out 😂

ultmaster · 2021-07-07T02:57:09Z

nni/retiarii/nn/pytorch/api.py

        elif isinstance(candidates, list):
            for i, module in enumerate(candidates):
                self.add_module(str(i), module)
                self.names.append(str(i))
+                if not self.chosen:


Why do we need a chosen? This seems a fix mode and should be done in __new__.

I need to call the first candidate. But as candidates is a list/dict, instead of ModuleList, so it can't be directly accessed in forward (eg., candidates[0]). I don't know if there are better ways to achieve that.

In that case, you can write: self._modules[self.names[0]](x)

In that case, you can write: self._modulesself.names[0]

Thanks, I got it.

In that case, you can write: self._modules[self.names[0]](x)

It seems I can't access self._modules in forward.

In that case, you can write self._first_module = self._modules[self.names[0]](x) in __init__. Might be more clear that self.chosen.
My previous concern is that some one-shot algorithms use layer choice directly and might treat self.chosen as another module due to wrong implementation. But I found that actually self.names is used in __iter__. So never mind...

ultmaster · 2021-07-07T06:39:23Z

nni/retiarii/converter/graph_gen.py

                cand_type = '__torch__.' + get_importable_name(cand.__class__)
-                graph.add_node(cand_name, cand_type, get_init_parameters_or_fail(cand))
+                graph.add_node(cand_name, cand_type, get_init_parameters_or_fail(cand, silently=True))
+                self._convert_module(script_cand, cand, cand_name, ir_model)


I think choices in LayerChoice should stop parsing. Please test with examples in https://github.com/microsoft/nni/tree/master/test/retiarii_test/darts to see if that works.

But nn-meter needs the subgraph of layerchoice's candidates.

If there are really some errors, users just need to wrap the candidate with serialize to stop parsing.

Actually serialize is required under the former implementation. There is no difference if serialize is provided. What I implement is that when the candidate is not wrapped with serialize, I will parse it recursively.

Make sense to me. If you have already tested the example, I'm okay with this change.

suggest to add ut for non-serialize case.

ultmaster · 2021-07-07T06:39:52Z

nni/retiarii/converter/graph_gen.py

+
+class HardwareAwareGraphConverter(GraphConverter):
+
+    def convert_module(self, script_module, module, module_name, ir_model, example_inputs):


Suggest adding docstring and unittests for this module.

…soft#3910)

…rt trace (microsoft#3910)

…t#3910)

kaleid-liner · 2021-07-11T06:46:22Z

I rebase and forced push to include multi-trial SPOS example (#3876) as later development is based on it.

ultmaster · 2021-07-12T02:11:51Z

nni/retiarii/strategy/filter.py

+from nn_meter import get_default_config, load_latency_predictors  # pylint: disable=import-error
+
+
+class LatencyFilter:


Suggest putting this filter into examples.

ultmaster · 2021-07-12T02:12:09Z

nni/retiarii/strategy/bruteforce.py

@@ -86,15 +86,28 @@ class Random(BaseStrategy):
        Do not try the same configuration twice. When variational is true, deduplication is not supported. Default: true.
    """

-    def __init__(self, variational=False, dedup=True):
+    def __init__(self, variational=False, dedup=True, model_filter=None):


Please update the docstring correspondingly.

ultmaster · 2021-07-12T02:14:58Z

nni/retiarii/experiment/pytorch.py

    # TODO: this logic might need to be refactored into execution engine
    if full_ir:
        try:
            script_module = torch.jit.script(base_model)
        except Exception as e:
            _logger.error('Your base model cannot be parsed by torch.jit.script, please fix the following error:')
            raise e
-        base_model_ir = convert_to_graph(script_module, base_model)
+        if parse_shape:


This looks like another execution engine.
I suppose you can merge parse_shape and example_inputs with full_ir, and rename full_ir to something like ir_format.

ultmaster · 2021-07-12T02:15:30Z

nni/retiarii/experiment/pytorch.py

@@ -171,7 +180,8 @@ def __init__(self, base_model: nn.Module, trainer: Union[Evaluator, BaseOneShotT

    def _start_strategy(self):
        base_model_ir, self.applied_mutators = preprocess_model(
-            self.base_model, self.trainer, self.applied_mutators, full_ir=self.config.execution_engine != 'py')
+            self.base_model, self.trainer, self.applied_mutators, full_ir=self.config.execution_engine != 'py',


Please add another value to execution_engine.

QuanluZhang · 2021-07-12T03:04:40Z

examples/nas/oneshot/spos/multi_trial.py

-    exp = RetiariiExperiment(base_model, trainer, [], simple_strategy)
+    example_inputs = torch.randn(1, 3, 32, 32)
+
+    base_model.eval()


it is a little strange to config dummy input and "eval" here, let's discuss in the meeting

Agree.
I think this input should be obtained from dataloader.

QuanluZhang · 2021-07-12T05:56:42Z

nni/retiarii/converter/graph_gen.py

+            the built graph ir from module, ```None``` means do not further parse the module
+        dict
+            the input arguments of this module
+        """


why wrap _convert_module with exactly the same input arguments? and seems convert_module does not have returns, but there are returns in the docstring

kaleid-liner · 2021-07-13T00:25:08Z

The tests failed because I move LatencyFilter to utils.py. The import from nn_meter import of this file incurs ImportError, as nn_meter hasn't been released,

ultmaster · 2021-07-13T04:58:11Z

nni/retiarii/strategy/utils.py

 from typing import Dict, Any, List
+from nn_meter import get_default_config, load_latency_predictors  # pylint: disable=import-error


Please move the filter to your example. This import will make nn_meter a "required" dependency of NNI. But I don't think it should be required.

ultmaster · 2021-07-13T09:24:29Z

nni/retiarii/experiment/pytorch.py

@@ -154,7 +160,8 @@ def debug_mutated_model(base_model, trainer, applied_mutators):

 class RetiariiExperiment(Experiment):
    def __init__(self, base_model: nn.Module, trainer: Union[Evaluator, BaseOneShotTrainer],
-                 applied_mutators: List[Mutator] = None, strategy: BaseStrategy = None):
+                 applied_mutators: List[Mutator] = None, strategy: BaseStrategy = None,
+                 parse_shape: bool = False, example_inputs = None):


Any chance we can put them into config?

QuanluZhang requested review from ultmaster and liuzhe-lz July 7, 2021 02:52

ultmaster reviewed Jul 7, 2021

View reviewed changes

QuanluZhang self-requested a review July 9, 2021 07:20

kaleid-liner added a commit to kaleid-liner/nni that referenced this pull request Jul 10, 2021

GraphConverter now support flatten the subgraphs to root graph (micro…

df4343f

…soft#3910)

kaleid-liner added a commit to kaleid-liner/nni that referenced this pull request Jul 10, 2021

Add imports to fix flake8 (microsoft#3910)

ac22a1f

kaleid-liner added a commit to kaleid-liner/nni that referenced this pull request Jul 10, 2021

GraphConverter now support flatten the subgraphs to root graph (micro…

007d021

…soft#3910)

kaleid-liner added a commit to kaleid-liner/nni that referenced this pull request Jul 10, 2021

Add imports to fix flake8 (microsoft#3910)

41ed419

kaleid-liner force-pushed the nn-meter branch from ac22a1f to 41ed419 Compare July 10, 2021 16:07

Jianyu Wei added 3 commits July 11, 2021 02:32

Fix mutable default

83b4bf8

LayerChoice:forward now will default run the first candidate to suppo…

35431e0

…rt trace (microsoft#3910)

New GraphConverter to parse shape info required by nn-meter (microsof…

f5bfc8c

…t#3910)

kaleid-liner force-pushed the nn-meter branch from 41ed419 to f5bfc8c Compare July 11, 2021 06:40

kalineid added 2 commits July 11, 2021 06:00

Support model filter in Random strategy

1a8bf45

Support latency aware search in SPOS multi-trial example

7a2a06c

ultmaster reviewed Jul 12, 2021

View reviewed changes

QuanluZhang reviewed Jul 12, 2021

View reviewed changes

kaleid-liner added 3 commits July 13, 2021 02:05

Fix for review (microsoft#3910)

2de4bff

Add doc for hardware-aware NAS

4b1d5ee

Fix lint python & Add nn_meter to sphinx mock

794521b

ultmaster reviewed Jul 13, 2021

View reviewed changes

ultmaster and others added 2 commits July 13, 2021 12:58

Add comments

d8f821a

Move LatencyFilter to examples

99c8fdc

ultmaster reviewed Jul 13, 2021

View reviewed changes

ultmaster added 2 commits July 14, 2021 14:53

Move example inputs into configs

385145c

Support nested layer choice

8dcbaac

ultmaster merged commit 99aa822 into microsoft:nn-meter Jul 15, 2021


		class HardwareAwareGraphConverter(GraphConverter):

		def convert_module(self, script_module, module, module_name, ir_model, example_inputs):

		from nn_meter import get_default_config, load_latency_predictors # pylint: disable=import-error


		class LatencyFilter:

		from typing import Dict, Any, List
		from nn_meter import get_default_config, load_latency_predictors # pylint: disable=import-error

[Retiarii]: Add info required by nn-meter to graph ir #3910

[Retiarii]: Add info required by nn-meter to graph ir #3910

Uh oh!

Conversation

kaleid-liner commented Jul 6, 2021

Uh oh!

ghost commented Jul 6, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaleid-liner Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ultmaster Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaleid-liner commented Jul 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaleid-liner commented Jul 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Jul 6, 2021 •

edited by ghost

Loading

kaleid-liner Jul 7, 2021 •

edited

Loading

ultmaster Jul 12, 2021 •

edited

Loading