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

Refactoring tpu_api_dlsym_initializer #55413

Closed
wants to merge 6 commits into from

Conversation

sshahrokhi
Copy link
Contributor

@sshahrokhi sshahrokhi commented Mar 28, 2022

We want to remove the static initializer of the TPU for JAX, which stays in the tpu_api_dlsym_initializer file. However, The functions and links in this file are needed to be called and linked in the correct places if we remove the static initializer. Therefore, we are only keeping the static initializer in this file, and have the tpu_initializer_helper file to have the rest of the functions and links. So there is no need to link this file in any place that does not need the static initializer. This PR does not change the behavior of anything, and is only refactoring functions.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Mar 28, 2022
@sshahrokhi
Copy link
Contributor Author

@skye could you please review this PR?

@skye skye self-requested a review March 28, 2022 23:28
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Mar 28, 2022
@skye skye added the ready to pull PR ready for merge process label Mar 29, 2022
@skye
Copy link
Member

skye commented Mar 29, 2022

Marking "ready to pull" so I can run internal tests, not actually ready for submission.

tensorflow/core/tpu/BUILD Show resolved Hide resolved
tensorflow/core/tpu/tpu_api_dlsym_initializer.cc Outdated Show resolved Hide resolved
tensorflow/core/tpu/tpu_initializer_helper.cc Outdated Show resolved Hide resolved
tensorflow/core/tpu/tpu_initializer_helper.cc Show resolved Hide resolved
tensorflow/core/tpu/tpu_initializer_helper.h Outdated Show resolved Hide resolved
tensorflow/core/tpu/tpu_initializer_helper.cc Show resolved Hide resolved
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Mar 29, 2022
@gbaned gbaned removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Mar 29, 2022
Copy link
Member

@skye skye left a comment

Choose a reason for hiding this comment

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

This looks good! Just one small comment, and I wanna check the internal tests before approving.

Also, can you test that this doesn't break TF? Since we don't have Cloud TPU presubmits (I think?).

tensorflow/core/tpu/tpu_initializer_helper.h Outdated Show resolved Hide resolved
PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Mar 29, 2022
@skye skye added the ready to pull PR ready for merge process label Mar 29, 2022
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 31, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 31, 2022
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 31, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 31, 2022
@gbaned gbaned added the kokoro:force-run Tests on submitted change label Apr 1, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 1, 2022
@gbaned
Copy link
Contributor

gbaned commented Apr 1, 2022

@sshahrokhi Can you please address Ubuntu Sanity errors? Thank you!

@gbaned gbaned removed the ready to pull PR ready for merge process label Apr 1, 2022
@sshahrokhi
Copy link
Contributor Author

sshahrokhi commented Apr 1, 2022

@gbaned the build seems to fail because of a mismatch in bazel version, I guess can we run them again to see if that is resolved?

@gbaned gbaned added the kokoro:force-run Tests on submitted change label Apr 4, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 4, 2022
@gbaned gbaned requested a review from skye April 5, 2022 12:44
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Apr 5, 2022
@sshahrokhi
Copy link
Contributor Author

@skye is out this week, @michaelbanfield could you please review this?

@michaelbanfield michaelbanfield self-requested a review April 5, 2022 22:44
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 5, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 5, 2022
copybara-service bot pushed a commit that referenced this pull request Apr 7, 2022
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Apr 7, 2022
@gbaned
Copy link
Contributor

gbaned commented Apr 7, 2022

Seems auto-merge is not happening but the changes are merged into master now, so we can close this. Thank you for the PR.

@gbaned gbaned closed this Apr 7, 2022
PR Queue automation moved this from Approved by Reviewer to Closed/Rejected Apr 7, 2022
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M CL Change Size: Medium
Projects
No open projects
PR Queue
  
Closed/Rejected
Development

Successfully merging this pull request may close these issues.

None yet

6 participants