-
Notifications
You must be signed in to change notification settings - Fork 18
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
Transforming Models from PyTorch/Tensorflow to POET #9
base: main
Are you sure you want to change the base?
Transforming Models from PyTorch/Tensorflow to POET #9
Conversation
created network transformation function for converting TensorFlow network layers to POET computation layers testing is done for ResNet model |
created network transformation function for converting PyTorch model graph to graph with POET computation layer nodes testing is done for ResNet18, ResNet50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @arnavsinghvi11 great job on the PR! It looks great in general. Left some minor comments. Let me know if you have any questions. Thanks again for this!!
|
||
|
||
# transforms input model's graph to output graph with POET layer nodes | ||
def graph_transform(traced): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve readability can we use type hints? As type torch.nn
as input and as type poet.DNNLayer
as expected output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function actually takes in the graph of a torch model and outputs a recompiled version of the graph with poet.DNNLayers. I printed the type of this graph which is: torch.fx.graph_module.GraphModule.new..GraphModuleImpl so do you still recommend including this as a type hint or would that stray away from readability?
# transforms input model's graph to output graph with POET layer nodes | ||
def graph_transform(traced): | ||
for n in traced.graph.nodes: | ||
if "<built-in function" in str(n.target): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add a line to clarify why we ignore built-in functions?
- If the torch layer is not part of POET's vocab, how do we handle it? One suggestion - a) we inform the user and if they confirm, b) we ignore the layer and continue given the input/output dimensionality is respected. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, would adding a print statement informing the layer is not supported by POET and continuing looping through suffice?
|
||
# transforms ResNet Model graph into POET layers nodes | ||
|
||
# #Resnet18 model transformation - https://github.com/pytorch/vision/blob/main/torchvision/models/resnet.py, commit: 7dc5e5bd60b55eb4e6ea5c1265d6dc7b17d2e917 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this commented for a reason? Good to have the tests in.
num_classes = 10 | ||
layers = [InputLayer((batch_size, *input_shape))] | ||
|
||
# comment out to output transformations for different ResNet models |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly confused by the comment message.
from poet.power_computation import InputLayer | ||
import tensorflow as tf | ||
|
||
# transforms VGG Model network layers into POET computation layers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment reads VGG Model but I think the ResNet model is being constructed here?
from poet.power_computation_transformer import QueryKeyValueMatrix, QKTMatrix, QKTVMatrix | ||
import torch.nn as nn | ||
import torchvision.models | ||
from torchvision.models.resnet import BasicBlock, Bottleneck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We say network transform, but this is mostly for Resnet+common layers? I think we should have a better way to organize this. I don't know exactly how - open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the goal was to extend this for other models like BERT which I didn't fully support in my implementation yet. I can comment out this import for now.
This PR is the first step towards translating models defined in Torch to Tensorflow transparently. We define network transformation functions which are then composed together for converting PyTorch and Tensorflow network layers to POET computation layers.
Tested for ResNet18, ResNet50, and VGG16.