Stop setting PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION env var #1887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephanwlee
approved these changes
Feb 25, 2019
|
Filed #1888 for making |
nfelt
added a commit
to nfelt/tensorboard
that referenced
this pull request
Feb 25, 2019
Merged
nfelt
added a commit
that referenced
this pull request
Feb 25, 2019
SA-PCR-RO
pushed a commit
to AnacondaRecipes/tensorflow_recipes
that referenced
this pull request
Mar 15, 2019
- tensorboard: drop protobuf patch. xref: tensorflow/tensorboard#1887 - update path for toco in workaround for linking problem on windows xref: tensorflow/tensorflow@61c6c84 - bazel doesn't play well with PATH in recent versions. Set CONDA_DLL_SEARCH_MODIFICATION_ENABLE=1 during tests - force bazel in host env on Windows since it has to be in the same env as the one in which all m2-* deps are installed - also build with cudatoolkit 10, cudnn 7.3 - add python 3.7 builds to the matrix for meta packages
jjhelmus
pushed a commit
to AnacondaRecipes/tensorflow_recipes
that referenced
this pull request
Apr 5, 2019
- tensorboard: drop protobuf patch. xref: tensorflow/tensorboard#1887 - update path for toco in workaround for linking problem on windows xref: tensorflow/tensorflow@61c6c84 - bazel doesn't play well with PATH in recent versions. Set CONDA_DLL_SEARCH_MODIFICATION_ENABLE=1 during tests - force bazel in host env on Windows since it has to be in the same env as the one in which all m2-* deps are installed - also build with cudatoolkit 10, cudnn 7.3 - add python 3.7 builds to the matrix for meta packages
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should fix #1566 by reverting #1489, which was unfortunately a misguided change on my part. I thought that the env var was necessary in order to opt in to the C++ implementation because not all protobuf python distributions have the C++ implementation activated by default even if they include the appropriate extension. Apparently this either was never true, or certainly hasn't been true since protobuf 3.5.1 (and we're already on 3.6.0). And setting the env var is actively harmful when protobuf doesn't include the C++ extension because then it fails with the error in #1566. (Protobuf should really provide a safe way to opt into the C++ implementation only if it is present, but they currently don't.)
I think I thought this was necessary because apparently when you
bazel build tensorboardthe resulting binary uses the bazel-providedprotobuf-pythonand that does seem to include the C++ extension but still default to the python implementation (which is why this env var made a difference in my limited initial testing; I just didn't realize I needed to test it against the pip package version).We should figure out a way to make the locally built
tensorboardfast that doesn't require setting this env vars unconditionally, but I'm fixing this first to unbreak users in #1566.Tested by building pip package and confirming that the selected protobuf implementation is still CPP and that when profiling event loading, it shows the C++ extension signatures in the output.