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.

Sign up for GitHub

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

Jump to bottom

C++ api: use of op::Attrs methods in gradients #17360

Closed
kbsriram opened this issue Mar 1, 2018 · 2 comments
Closed

C++ api: use of op::Attrs methods in gradients #17360

kbsriram opened this issue Mar 1, 2018 · 2 comments
Labels
stat:awaiting tensorflower Status - Awaiting response from tensorflower type:bug Bug

Comments

@kbsriram
Copy link
Contributor

kbsriram commented Mar 1, 2018

The generated op::Attrs struct returns new instances on its chainable methods, and doesn't change the original object.

tensorflow/tensorflow/cc/framework/cc_op_gen.cc

Line 701 in d7d7f4e

strings::StrAppend(&setters, " Attrs ret = *this;\n");

There are a few related issues e.g.

tensorflow/tensorflow/cc/gradients/nn_grad.cc

Line 164 in 6fdb9ad

grad_attrs.DataFormat(data_format);
where the code assumes the underlying object is being mutated and the parameters don't actually pass through.

I guess there might be a couple of ways forward, depending on how Tensorflow prefers the C++ API:

  1. Decide the Attrs chaining methods mutate the underlying object and fix the code generation.
  2. Decide the Attrs chaining methods return new instances, and fix the uses.

Suggestions?

Fwiw if option 2, it might be nice to add TF_MUST_USE_RESULT to the generated API. (Unfortunately a long-standing bug in gcc means this may be unreliable as an actual error across versions of gcc that contributors may use.)

/cc @suharshs @keveman
@keveman
Copy link
Contributor

keveman commented Mar 1, 2018

@kbsriram thanks for pointing out the bug. I am not a fan of mutable state, so my natural inclination is towards option 2. Adding TF_MUST_USE_RESULT to the generated API would simply point out the existing erroneous uses as compiler errors.

@angerson angerson added stat:awaiting tensorflower Status - Awaiting response from tensorflower type:bug Bug labels Mar 1, 2018
@kbsriram
Copy link
Contributor Author

kbsriram commented Mar 1, 2018

@keveman thanks for the note! Sgtm - don't have a strong opinion on this. But would like to make progress on fixes and (for option 2 as you note) add a bit of a compile-time safety net for new code.

Tentatively predicated on option 2, suggestions on next steps? If someone is already on it, awesome - otherwise, I can sign up for a pull request on this.

@yifeif yifeif closed this as completed in 5279cf2 Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stat:awaiting tensorflower Status - Awaiting response from tensorflower type:bug Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@keveman @kbsriram @angerson