Skip to content

Generate op input and attribute accessors #302

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

Merged
merged 25 commits into from
Oct 15, 2021

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Apr 27, 2021

This adds a nested class to the generated op classes that contains typesafe (and not quite type safe) accessors for the op inputs and attributes. It's only supported for graph ops at the moment, but it will eventually support ForwardOperations as well (from the eager gradient support). The main use for this is defining custom gradients, whether it's the legacy graph gradients or the new gradient API.

I haven't committed the generated outputs to make it easier to review, I'll do so after approval or in another PR.

@rnett rnett changed the title Generatr op input and attribute accessors Generate op input and attribute accessors Apr 27, 2021
@rnett
Copy link
Contributor Author

rnett commented Apr 27, 2021

The formatting issues were since my IDE somehow got off google-java-format. I have the plugin now, which added some things that weren't in the XML.

@rnett rnett force-pushed the rn_generatr_op_inputs branch from 002fa85 to faca5f4 Compare April 27, 2021 20:17
@karllessard
Copy link
Collaborator

Looks like the javadoc reformatting is still present

@rnett
Copy link
Contributor Author

rnett commented Apr 28, 2021

Yeah, that happened when I applied the google-java-format plugin, and it matches the spec. Idk why the XML format didn't do it, I can revert to that.

We really should set up some kind of CI formatter, I keep getting burnt by this. I haven't had time to look at it much but one of the plugins you linked in the lint PR looked like it did ktlint too (although it had a bug), that would be nice since we could to the Kotlin API in the same plugin.

rnett and others added 19 commits September 18, 2021 16:09
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
Signed-off-by: Ryan Nett <[email protected]>
@rnett rnett force-pushed the rn_generatr_op_inputs branch from a32b212 to 0895e48 Compare September 18, 2021 23:15
@rnett
Copy link
Contributor Author

rnett commented Sep 24, 2021

Reminder for myself: run generation before merging! I'm going to work on a CI test for that.

@rnett
Copy link
Contributor Author

rnett commented Sep 28, 2021

FYI @karllessard I'm going to be out of town most of the next 2 weeks or so. I might have time to follow up on these PRs, but I'm not sure. Feel free to take things over. I should at least be able to reply to comments.

@rnett rnett requested a review from karllessard October 14, 2021 16:12
karllessard
karllessard previously approved these changes Oct 15, 2021
Copy link
Collaborator

@karllessard karllessard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rnett !

@karllessard
Copy link
Collaborator

@rnett , can you please fix the conflict with Names so I can merge?

@rnett
Copy link
Contributor Author

rnett commented Oct 15, 2021

Let me generate the op files first, too.

@rnett
Copy link
Contributor Author

rnett commented Oct 15, 2021

I've been toying with the idea of a CI check that checks git status after running build to ensure that there's no un-commited codegen, thoughts?

@karllessard
Copy link
Collaborator

I've been toying with the idea of a CI check that checks git status after running build to ensure that there's no un-commited codegen, thoughts?

That's interesting, indeed! Shouldn't be hard to do. Though it looks like we still have this non-deterministic order of the *Ops fields in the main Ops class, or at least I do. We'll need to fix that first.

@rnett
Copy link
Contributor Author

rnett commented Oct 15, 2021

Yeah, good point, I'll look at that first.

@karllessard
Copy link
Collaborator

Looks good, I've just merged another PR (#388) though so I'll wait that one to complete building before merging this one.

@karllessard karllessard merged commit 05d9de3 into tensorflow:master Oct 15, 2021
@karllessard
Copy link
Collaborator

karllessard commented Oct 15, 2021

@rnett , I've just realized that we forgot to a CI build round on this PR before merging it... :(

Failing on Windows: https://github.com/tensorflow/java/runs/3908491275?check_suite_focus=true

It fails on Linux too but it looks more like a bug we are having with the Bazel cache and that was happening before.

CC\ @saudet

@rnett
Copy link
Contributor Author

rnett commented Oct 15, 2021

I think I know what that is, I just need to skip a bunch of things that aren't exported on windows.

@rnett rnett deleted the rn_generatr_op_inputs branch October 17, 2021 00:42
@karllessard karllessard mentioned this pull request Oct 20, 2021
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.

3 participants