Skip to content
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

Initial support for AddV2 #181

Merged
merged 1 commit into from
Oct 15, 2019
Merged

Initial support for AddV2 #181

merged 1 commit into from
Oct 15, 2019

Conversation

hrydgard
Copy link
Contributor

@hrydgard hrydgard commented Oct 15, 2019

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

@kali kali merged commit d6779c3 into sonos:master Oct 15, 2019
@kali
Copy link
Collaborator

kali commented Oct 15, 2019

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.

@hrydgard
Copy link
Contributor Author

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 :)

@kali
Copy link
Collaborator

kali commented Oct 15, 2019

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.

@kali
Copy link
Collaborator

kali commented Oct 15, 2019

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.

@hrydgard
Copy link
Contributor Author

Yeah, that should do nicely!

@hrydgard hrydgard deleted the support-addv2 branch October 16, 2019 07:16
@hrydgard
Copy link
Contributor Author

hrydgard commented Oct 16, 2019

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...

https://git.codingcafe.org/Mirrors/tensorflow/tensorflow/commit/1b6b7e208f22b2f15768464e266f0fc4c235b4de

I think it'll eventually show up in official ONNX... But adding it at runtime will work for us in the meantime!

@kali
Copy link
Collaborator

kali commented Oct 16, 2019

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.

@hrydgard
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing node type: AddV2
2 participants