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

Upgrade for TensorFlow 2.5.0 #303

Merged
merged 10 commits into from
Jun 10, 2021

Conversation

saudet
Copy link
Contributor

@saudet saudet commented Apr 27, 2021

I was thinking of doing this as part of pull #283, but there are too many changes to the ops, so let's do that separately. The tests pass so it's kind of working, but if we remove the src/gen directory, a lot of ops don't get regenerated. Also, as you can see from the second commit, there is a lot of new stuff that doesn't look right.

BTW, this is still only a RC upstream, so if we find issues fast enough, we should be able to have them fixed before the release!
@karllessard @rnett

@rnett rnett mentioned this pull request Apr 27, 2021
@rnett
Copy link
Contributor

rnett commented Apr 27, 2021

The best way to track op differences is to use #305. I compared the files and there doesn't seem to be anything too out of the ordinary. Just standard op changes.

FYI the op generator deletes the base directory (gen/java/org/tensorflow/op) when you run it, so I don't know what's up with that. It's deterministic based on ops.pb.

@saudet
Copy link
Contributor Author

saudet commented Apr 28, 2021

Ok, so why doesn't everything get regenerated when purging the src/gen directory?

@rnett
Copy link
Contributor

rnett commented Apr 28, 2021

Try just purging the org.tensorflow.op package, I think the rest is just protobuf? I'll clone your branch and check, but I can't think of a reason why ops wouldn't be generated.

@rnett
Copy link
Contributor

rnett commented Apr 29, 2021

Deleting and re-generating src/gen/java/org/tensorflow/op works fine for me, it's the exact same thing.

@saudet
Copy link
Contributor Author

saudet commented Apr 29, 2021

Ah, you're right, it looks like I simply forgot to rerun the whole build. It didn't work with the old generator though. Now, it's actually just a couple of proto classes that are gone. So, this looks alright after all.

@saudet
Copy link
Contributor Author

saudet commented Apr 29, 2021

Ok, so, well, this looks good to me. Please take a look and feel free to merge! @karllessard

@Craigacp
Copy link
Collaborator

2.5.0 came out yesterday (https://github.com/tensorflow/tensorflow/releases/tag/v2.5.0), could you bump this PR to use that? Then we can get it merged in.

@saudet saudet force-pushed the upgrade-tensorflow-250 branch from 8249e31 to 48755af Compare May 17, 2021 00:45
@karllessard
Copy link
Collaborator

As usual, when upgrading the version of TF, we need to run the API importer to classify properly the new ops (which are all added to the root package by default).

The process is not fully straightforward and even a bit arbitrary. I'll try to find some time soon to run it but I guess I should document what I'm doing so that others can also do it (and maybe even better!). I'll push my changes to your branch @saudet , can you please grant me write access to it if I don't have them already?

@saudet
Copy link
Contributor Author

saudet commented May 18, 2021

You should already have write access, yes. Let me know if you don't see the "Add more commits by pushing to the..." message below the comments here though.

@google-cla
Copy link

google-cla bot commented Jun 8, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@karllessard
Copy link
Collaborator

@googlebot I consent.

@karllessard karllessard force-pushed the upgrade-tensorflow-250 branch 3 times, most recently from dc56bcb to 9da472e Compare June 8, 2021 17:14
@karllessard karllessard force-pushed the upgrade-tensorflow-250 branch from 9da472e to a0ae39e Compare June 8, 2021 17:20
@karllessard
Copy link
Collaborator

@saudet I'm done classifying the new ops and protos in TF2.5 but we are hitting an error in the MacOSX CI build, any chance you can take a look at it? I think it happens here:

Usage: /Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/install_name_tool [-change old new] ... [-rpath old new] ... [-add_rpath new] ... [-delete_rpath old] ... [-id name] input

@karllessard
Copy link
Collaborator

I've just disabled (maybe temporarily) this previous path fix, which seems to cause the error. We'll see how the CI build goes.

@karllessard
Copy link
Collaborator

As the SIG has already discussed during our last session, we'll disable for now MKL builds because they are unstable and causing us too much trouble to build, while seemingly not providing any improvement in the performances (and actually, quite the opposite).

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.

4 participants