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

Fix MPI build failure caused by StringPiece -> absl::string_view #22084

Merged

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Sep 5, 2018

This fix tries to fix the MPI build failure caused by StringPiece -> absl::string_view.

This fix fixes #22376.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

This fix tries to fix the MPI build failure caused by
StringPiece -> absl::string_view.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@martinwicke
Copy link
Member

@jlebar have we caught these already?

@jlebar
Copy link
Contributor

jlebar commented Sep 8, 2018

@jlebar have we caught these already?

Looks to me like we have not.

@yongtang yongtang added the kokoro:force-run Tests on submitted change label Sep 19, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 19, 2018
@yongtang
Copy link
Member Author

Updated the description to include This fix fixes #22376., as issue #22376 is related.

/cc @jlebar @martinwicke

@jlebar
Copy link
Contributor

jlebar commented Sep 19, 2018

Looks like you clang-formatted the whole file. That's not the right thing to do: clang-format is not a stable format, so if you clang-format the entirety of every file you touch, we're going to introduce a lot of irrelevant whitespace changes.

The correct thing is to apply the diff that the clang-format test was saying to apply, or to use the git-clang-format tool that you can find in the LLVM repository, e.g. https://llvm.org/svn/llvm-project/cfe/trunk/tools/clang-format/git-clang-format.

Whitespace, I know...

@yifeif IIRC we're now clang-formatting patches when we import them to be submitted. Does that allow us to turn off the external clang-format check, so we don't have to go through this with folks anymore?

…ang-format Check`

The following chanes has been applied to align with `Experimental clang-format Check` test:
```
diff --git a/tensorflow/contrib/mpi/mpi_rendezvous_mgr.cc b/tensorflow/contrib/mpi/mpi_rendezvous_mgr.cc
index e195cca..b9967fe 100644
--- a/tensorflow/contrib/mpi/mpi_rendezvous_mgr.cc
+++ b/tensorflow/contrib/mpi/mpi_rendezvous_mgr.cc
@@ -136,8 +136,8 @@ void MPIRemoteRendezvous::RecvFromRemoteAsync(

   MPIRendezvousMgr* mgr =
       reinterpret_cast<MPIRendezvousMgr*>(this->rendezvous_mgr_);
-  mgr->QueueRequest(string(parsed.FullKey()), step_id_,
-                    std::move(request_call), rendezvous_call);
+  mgr->QueueRequest(string(parsed.FullKey()), step_id_, std::move(request_call),
+                    rendezvous_call);
 }

 MPIRemoteRendezvous::~MPIRemoteRendezvous() {}
```

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Copy link
Member Author

Thanks @jlebar. I have updated the PR and now only applied the clang-format change as is shown in Experimental clang-format Check. Please take a look and let me know if there are any issues.

@phalexo
Copy link

phalexo commented Sep 19, 2018

So was MPI related ToString issue addressed? I still see it with the latest master pull.

@jlebar
Copy link
Contributor

jlebar commented Sep 19, 2018

So was MPI related ToString issue addressed? I still see it with the latest master pull.

Isn't the fix you're looking for in this PR, which is still unsubmitted?

@phalexo
Copy link

phalexo commented Sep 19, 2018

So was MPI related ToString issue addressed? I still see it with the latest master pull.

Isn't the fix you're looking for in this PR, which is still unsubmitted?

I don't know. I guess I was hoping that my question might prod someone to submit it. Wishful thinking?

@yongtang
Copy link
Member Author

@phalexo This PR addresses the issue you encounter. The PR is not merged into the master yet, but it will go into master soon.

@jlebar
Copy link
Contributor

jlebar commented Sep 19, 2018

This is ready for @martinwicke to submit.

@martinwicke
Copy link
Member

@jlebar, in that case, just add the "ready to pull" label.

@martinwicke martinwicke added the ready to pull PR ready for merge process label Sep 19, 2018
@martinwicke
Copy link
Member

(just did for this one)

@tensorflow-copybara tensorflow-copybara merged commit c8b376a into tensorflow:master Sep 21, 2018
tensorflow-copybara pushed a commit that referenced this pull request Sep 21, 2018
@yongtang yongtang deleted the 09012018-mpi-StringPiece branch September 21, 2018 22:19
@EFanZh
Copy link
Contributor

EFanZh commented Sep 22, 2018

What about r1.11 branch? Is it going to get the patch?

@yongtang
Copy link
Member Author

@EFanZh I created a PR #22474 to cherry pick the fix on r1.11 branch. As r1.11 is already in release candidate 2, the PR might not be accepted though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to build TF from source with MPI support
8 participants