[WIP] Add framework for version converter API #1926
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| ) | ||
| model = ir.serde.deserialize_model(model_proto) | ||
| self.assertEqual(model.graph._nodes[4].op_type, "GridSample") | ||
| self.assertEqual(model.graph._nodes[4]._attributes['mode'].value, 'bilinear') |
There was a problem hiding this comment.
| self.assertEqual(model.graph._nodes[4]._attributes['mode'].value, 'bilinear') | |
| self.assertEqual(model.graph[4].attributes['mode'].value, 'bilinear') |
❌ 15 Tests Failed:
View the full list of 3 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
| ### Adapters | ||
|
|
||
| # Compatibility Adapter | ||
| def adapter_compatible(op: ir.Node, target_opset): |
There was a problem hiding this comment.
nit: I recommend node instead of op. We frequently use op for building a node via the op.MatMul(x, y) syntax.
There was a problem hiding this comment.
Furthermore, the general interface for a version-converter would likely need an op/node builder as an input, and that would be best called op. We would need that when creating new nodes as part of the version-conversion (eg., even in the example below to create a Constant node from an attribute value.
|
|
||
|
|
||
| _ADAPTERS_18_19 = { | ||
| "Equal": adapter_compatible, |
There was a problem hiding this comment.
Minor nit: I think it would be better to not have to register an adapter for compatible extensions, since it is basically an identity operation.
I can see a value in explicitly documenting that a particular opset-update is backwards-compatible (to catch the case where we forget to register an adapter). We could do that using a separate set of all compatible ops.
|
|
||
| # Compatibility Adapter | ||
| def adapter_compatible(op: ir.Node, target_opset): | ||
| op.version = target_opset |
There was a problem hiding this comment.
Setting the version on a node need not be part of the adapter logic. It should be in the converter logic that calls the adapter, since this logic is the same for all adapters. No point in duplicating this line in every adapter. That would also make a compatible adapter an identity function, and we don't even need to register one for compatible extensions,
|
One of the main design question relates to the "adapter" signature: what form should it take? Essentially it is a function that takes a single node as a parameter, and modifies it in some form. The changes are typically a simple mutation of a node along with potentially other changes (such as the insertion of extra nodes). For now, I think it might be fine to follow the pattern used in the optimizer and rewriter, which are based on node-transformers that, given an input node, return a sequence of replacement nodes or None (if no replacement is required). This allows a simple loop over all nodes in the graph that transforms each node in sequence. This can be generalized later if necessary. |
| name=_attr.name, | ||
| ) | ||
| # Add the ir.Value as inputs to the node and graph | ||
| node._inputs = node._inputs + (attr_as_input,) |
There was a problem hiding this comment.
note: avoid using and modifying private fields. To change inputs to a node, always initialize a new node to replace the current one.
There was a problem hiding this comment.
+1 ... we could do what the optimizer and rewriter currently do: use a more generic interface for a node-adapter that returns a list of replacement nodes (or None if no replacement is needed).
There was a problem hiding this comment.
Further, we can't just create a value as above ... we need to create a constant value with the given value. For now, the simplest way is to create a new Constant node. (Orthogonally to this, we should extend the "builder" API we currently use to create initializers as well, but that's a separate issue, for now Constant nodes should be fine.)
| self.assertEqual(nodes[1].version, 19) | ||
| self.assertEqual(nodes[4].op_type, "GridSample") | ||
| self.assertEqual(nodes[4].version, 20) | ||
| self.assertEqual(model.graph._nodes[4]._attributes["mode"].value, "cubic") |
There was a problem hiding this comment.
note: Avoid accessing internal fields
shubhambhokare1
added
enhancement
| self.custom_adapters = custom_adapter_list | ||
|
|
||
| def graph_version_convert(self, graph: ir.Graph, target_version: int) -> None: | ||
| if self.model_version == target_version: |
There was a problem hiding this comment.
(Extension) I think we will need to soon support the case where the incoming model has nodes with different opset versions. At that point, such checks should happen at the node level, not at the model level.
|
|
||
| # Iterate from current model version -> target version | ||
| # Updating each node based on the correct adapter [ver->ver+1] | ||
| for opset_version in range(self.model_version, target_version): |
There was a problem hiding this comment.
May be better to explicitly check for target_version being > or < current version. (It's ok to focus on up-conversion in first version, but may be better to have an error/warning message if we run into down-conversion when it is unimplemented.)
There was a problem hiding this comment.
Actually, this could be bundled into a check after pick_adapter_set to more generally handle the case when we don't have an adapter set (for either 23 to 24 or for 18 to 17).
|
Questions that are related to the design doc that comes to mind
|
| def __init__(self, target_version: int): | ||
| self.target_version = target_version | ||
|
|
||
| def process_node(self, node: ir.Node, opset_version): |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
| model = ir.serde.deserialize_model(model_proto) | ||
| target_version = 17 | ||
| version_converter.convert_version(model, target_version=target_version) | ||
| nodes = model.graph._nodes |
Check notice
Code scanning / CodeQL
Unused local variable Note
No description provided.