-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Initial support for AddV2 #181
Conversation
Aha, just for the record and for next time, it did not have to be added to the onnx crate. I merged the PR a tidy bit fast, and it should not break anything, so I'll fix it directly on the master branch. |
I am actually using this on ONNX files exported from Tensorflow though through a somewhat hacky flow - but sure if it's not needed for that, even, go ahead :) |
Ha, so my fix on master may break it again for you. I would prefer we avoid polluting the Onnx namespace by importing Tensorflow crazyness, but I have a workaround to register more ops from the API. Tell me if you need it. |
The gist of it, if anybody needs it... let mut onnx = tract_onnx::onnx();
onnx.op_register.insert("AddV2", |_, _| Ok((Box::new(tractops::math::add::bin()), vec![]))); and then proceed with onnx as usual. |
Yeah, that should do nicely! |
Finally figured out the point of AddV2, it is really the same as regular Add, just defined a bit looser (is_associative, is_aggregate) so that graph optimizers can do more with it. The old Add was defined not to be, just because it supported strings (!) and adding those is not commutative... I think it'll eventually show up in official ONNX... But adding it at runtime will work for us in the meantime! |
Ha, thanks for investigating that. tract always assumed Add is associative and commutative anyway. I know it's not strictly true for floats, but I don't intend to depart from that untill I'm shown a real-life model that does not work under this assumption. To be honest, I doubt very much that Onnx will include AddV2, as it's current Add is very much in line with AddV2. The long term solution is probably more to fix the tf-to-onnx translation process to map AddV2 to Add. |
Hm, yeah, you're probably right that it's the converter that will change. I'll later look into updating the converter we are using, but in the meantime, thanks for the registration trick! |
AddV2 is a new operator that Tensorflow 2 likes to emit. As per the documentation it is identical to Add. Compare https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/add-v2 to https://www.tensorflow.org/api_docs/cc/class/tensorflow/ops/add ...
I don't understand what the point of this op is, but this simple workaround fixes my problem :)
Could also of course duplicate the functions, but until I can find information about what makes AddV2 different, I think this is better...
Fixes #180