Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Retiarii] New API: Repeat and Cell #3481

Merged
merged 17 commits into from
May 26, 2021

Conversation

ultmaster
Copy link
Contributor

@ultmaster ultmaster commented Mar 26, 2021

This PR proposes two new high-level APIs for Retiarii.

  • API design
  • Mutation support
  • Test strategies

What will NOT be included in this PR:

  • One-shot algorithms support
  • Graph-aware algorithms

@ultmaster ultmaster marked this pull request as draft March 26, 2021 07:07
@kvartet kvartet mentioned this pull request Apr 26, 2021
49 tasks
@ultmaster ultmaster added this to In progress in v2.3 Apr 27, 2021
@ultmaster ultmaster self-assigned this Apr 27, 2021
@ultmaster ultmaster changed the title [Retiarii] New API: VariableSequential and Cell [Retiarii] New API: Repeat and Cell May 20, 2021
@ultmaster ultmaster marked this pull request as ready for review May 20, 2021 08:16
@ultmaster ultmaster moved this from In progress to Review in progress in v2.3 May 21, 2021
return x


class Cell(nn.Module):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "cell" could be more general.
op_candidates and num_nodes (or num_ops maybe) are likely to be supported by almost all algorithms, but ops_per_node and merge_op might not.
And from end-user's perspective, without knowledge about the NAS algorithm, they might not understand why there are "ops per node".
My suggestion is, either make optional parameters kwargs, or only support basic params in Cell and let algorithms to inherit it.
I do accept current version if it's not considered API freeze.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the term is widely accepted in NAS literature. This paragraph is taken from a recent literature by Facebook AI.

image

I did survey a few more papers and they all used the term "cell".

We can think of a better name though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, it is a little confusing that there are two Cells, another one is in our graph ir

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name wanted. Feel free to propose a new one.

blocks: Union[Callable[[], nn.Module], List[Callable[[], nn.Module]], nn.Module, List[nn.Module]],
depth: Union[int, Tuple[int, int]], label=None):
super().__init__()
self._label = label if label is not None else f'valuechoice_{uid()}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valuechoice_{uid()}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

self.min_depth = depth if isinstance(depth, int) else depth[0]
self.max_depth = depth if isinstance(depth, int) else depth[1]
assert self.max_depth >= self.min_depth > 0
if not isinstance(blocks, list):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the meaning of "list of module"? users replicate module to max_depth by themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

# Support loose end concat (shape inference on the following cells)
# How to dynamically create convolution with stride as the first node

def __init__(self,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems it is better to put this Cell class into search space zoo/hub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss on teams.

@@ -76,6 +76,40 @@ def mutate(self, model):
target.update_operation(target.operation.type, {**target.operation.parameters, argname: chosen_value})


class RepeatMutator(Mutator):
def __init__(self, nodes: List[Node]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to explain the meaning of nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

for edge in rm_node.outgoing_edges:
edge.remove()
rm_node.remove()
model.get_node_by_name(node.name).update_operation(Cell(node.operation.cell_name))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the meaning of this line, why it is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To delete unused parameters. Otherwise, codegen will complain.

v2.3 automation moved this from Review in progress to Reviewer approved May 25, 2021
@ultmaster ultmaster merged commit 5df75c3 into microsoft:master May 26, 2021
v2.3 automation moved this from Reviewer approved to Done May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v2.3
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants