-
Notifications
You must be signed in to change notification settings - Fork 419
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
PyTorch: merge layer with a constant input #1082
Comments
Hi! Thanks for testing this and working on a fix! I think your development goes into a promising direction. From a quick check of your fork, it seems to be like hsl4ml is still treating the new Constant layer like an input layer and is trying to find it's tensor at runtime. I'm not quite sure how best to fix it, but maybe @vloncar can advise. |
A similar situation came up a while ago for RNG feature. I think the best way would be to introduce a new type of node that has no input, only output. Then we can use it as a constant input in situations like these. It would require some changes in how this is handled transparently to the rest of the nodes in the graph. |
Thanks for the comment. Let me know if I can help out. For the time being I am adding additional input tensors and passing in constant values to them. |
Note, we do this lots in the ONNX parsing, mostly to handle Quant nodes between the weights and where they are used. See if maybe the Constant node we have for ONNX parsing is useful here. |
That new Constant node does look like it would be very useful for this use case. I'll play with that. |
Yep, that seems to work. I think the I have now an implementation that seems to work and that makes use of the |
Oh sweet. I'll give it a shot next week. Thanks! |
@JanFSchulte I'm testing this today. This update #1121 wasn't in your fork so I needed to manually add it for my case. Just FYI. I'll report back soon on whether it worked for me. |
@JanFSchulte your fork worked for my test! Thanks so much! |
Great, thanks for checking! PR with the fix is here: #1123 |
@JanFSchulte I think there might be an issue when multiple constants are present. For example, for the following class I get the same failure as before.
|
Ah damn, didn't think to test that. I'll have a look and update the PR accordingly. |
The issue was that pytorch starts adding |
Ok, I confirmed it works on that small test case. I'll next try my more complicated network. |
Seems to be working for my resnet. Thanks! |
Prerequisites
Please make sure to check off these prerequisites before submitting a bug report.
Quick summary
Merge layers that have a constant input do not work with PyTorch.
Details
Steps to Reproduce
Expected behavior
Successful synthesis.
Actual behavior
Optional
Possible fix
I figured out that the main reason this is happening is because the constant gets embedded in the
torch.fx
node representing themul
. I have been working on a fix that includes looking for constants and adding input like layers for the constants. I have gotten it to the point where the constant is represented as a layer, but haven't yet been able to get the actual constant through. I have to put it down for a few days, so I thought I'd post this to see if you think I'm on the right track. My fork is here if someone wants to have a look.The text was updated successfully, but these errors were encountered: