TPC attach 2 framework #1296
TPC attach 2 framework #1296
Conversation
…'attach' method that returns a TPC
| @@ -61,7 +61,6 @@ class OperatorSetNames(Enum): | |||
| OPSET_DROPOUT = "Dropout" | |||
| OPSET_SPLIT = "Split" | |||
| OPSET_CHUNK = "Chunk" | |||
| OPSET_UNBIND = "Unbind" | |||
There was a problem hiding this comment.
@lior-dikstein ignore this, since you already remove this in your PR
| } | ||
|
|
||
| if FOUND_SONY_CUSTOM_LAYERS: | ||
| self._opset2layer[OperatorSetNames.OPSET_POST_PROCESS] = [SSDPostProcess] |
There was a problem hiding this comment.
@lior-dikstein assuming this is the operator set name that is going to be added to the enum for pre process
| OperatorSetNames.OPSET_SUB.value: [tf.subtract, Subtract], | ||
| OperatorSetNames.OPSET_MUL.value: [tf.math.multiply, Multiply], | ||
| OperatorSetNames.OPSET_DIV.value: [tf.math.divide, tf.math.truediv], | ||
| OperatorSetNames.OPSET_MIN_MAX.value: [tf.math.minimum, tf.math.maximum, Minimum, Maximum], |
There was a problem hiding this comment.
Split two operator min & max
There was a problem hiding this comment.
@lior-dikstein this is more relevant to your PR
| @@ -9,3 +9,4 @@ | |||
| OperatorSetConcat= schema.OperatorSetConcat | |||
| Fusing = schema.Fusing | |||
| TargetPlatformModel = schema.TargetPlatformModel | |||
There was a problem hiding this comment.
I added this in my PR
| OperationsSetToLayers | ||
|
|
||
|
|
||
| class TpcAttach2Fw: |
There was a problem hiding this comment.
I perfer the name "AttachTPModelToFramework" or "AttachTpModelToFw"
"AttachTpModelToPytorch"/"Keras"
There was a problem hiding this comment.
I wanted it to begin with TPC to put it into context, I think it's clearer, but I wouldn't mind changing it
| # operation set are provided separately in the mapping. | ||
| self._opset2attr_mapping = None | ||
|
|
||
| def attach(self, tpc_model: TargetPlatformModel, |
There was a problem hiding this comment.
why "tpc_model" and not "tp_model"?
There was a problem hiding this comment.
this is debatable, but @haihabi want us to refer to what currently known as "TP model", meaning the platform description that is going to be given as a JSON, as "TPC model".
so I'm starting to change the terminology inside the MCT
| tpc = TargetPlatformCapabilities(tpc_model) | ||
|
|
||
| with tpc: | ||
| for opset_name, operators in self._opset2layer.items(): |
There was a problem hiding this comment.
don't we want to eliminate the context manager functionality in this new implementation?
There was a problem hiding this comment.
I don't mind keeping it for now in the TPC initialization because it is not part of the schema and I prefer as minimal unessential changes that can break things
| # its framework-specific attribute name. | ||
| # note that a DefaultDict should be provided if not all the layer types in the | ||
| # operation set are provided separately in the mapping. | ||
| self._opset2attr_mapping = None |
There was a problem hiding this comment.
add comments:
self._opset2layer = None # Mapping of operation sets to their corresponding framework-specific layers
# A mapping that associates each layer type in the operation set (with weight attributes and a quantization configuration in the target platform model) to its framework-specific attribute name. If not all layer types in the operation set are provided in the mapping, a DefaultDict should be supplied to handle missing entries.
| if attr_mapping is None: | ||
| OperationsSetToLayers(opset_name, operators) | ||
| else: | ||
| OperationsSetToLayers(opset_name, operators, attr_mapping=attr_mapping) |
There was a problem hiding this comment.
Why do you need the "if"? Just do
OperationsSetToLayers(opset_name, operators, attr_mapping=attr_mapping
| def attach(self, tpc_model: TargetPlatformModel, | ||
| custom_opset2layer: Dict[str, Tuple[List[Any], Optional[Dict[str, DefaultDict]]]] = None | ||
| ) -> TargetPlatformCapabilities: | ||
|
|
There was a problem hiding this comment.
Need to add documentation
|
|
||
| def __init__(self): | ||
| self._opset2layer = None | ||
|
|
There was a problem hiding this comment.
why not get it as an input and eliminate the context manager functionality? then you also document it with type hint and documentation
There was a problem hiding this comment.
As I said, I prefer not to deal with the TPC context manager elimination as part of this refactor
Pull Request Description:
Implementing TPC Attach2Fw module to attach a given TPC model to the framework-specific operators.
The attachments is based on a hardcoded mapping between the TPC schema's OperatorSetNames enum to each framework layers and ops (keras/pytorch).
Note that current TPCs do not use this module, since they will be removed and replaced with a simpler initialization framework in the future. So currently, this module is not used in MCT but it is just a preparation for future modifications.
Checklist before requesting a review: