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

summary: avoid using 'tensorflow' package in import logic #3515

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

nfelt
Copy link
Contributor

@nfelt nfelt commented Apr 14, 2020

The logic in tensorboard/summary/_tf/summary/__init__.py has a routine where it searches a bunch of possible TF package names in sys.modules in order to find the v2 summary API symbols that it needs to re-export. This can have a false positive for the first package - tensorflow itself - since depending on the build configuration, i.e. when building with --define=tf_api_version=1, this package still contains the v1 rather than v2 API. We had attempted to guard against this issue by only using tensorflow when tf.__version__ was 2.x, but this check isn't sufficient because it's still possible to build 2.x TF versions with --define=tf_api_version=1. In that case, this logic would mistakenly try to import the symbols from tensorflow, resulting in a bunch of deprecation warnings and not actually managing to re-export the critical symbols like create_file_writer().

Rather than attempting to fix this check, we just avoid ever trying to use tensorflow. The other package names we try all have "v2" in the package name so I don't think the same issue can affect them; they will always contain the v2 API. This change might mean that we fall back to using _api packages more often, but while that's ugly, we're already doing it in some of the cases depending on where within the TF API hierarchy the re-exporting logic gets first triggered, so it's not really a change from the status quo.

Tested by patching this change internally and confirming it addresses the issues we were seeing; the existing pip package smoke tests should suffice for OSS.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Thank you.

I glanced internally and couldn’t find a bug to reference (just an email
thread), so this is fine.

@nfelt nfelt merged commit 2c22dae into tensorflow:master Apr 14, 2020
@nfelt nfelt deleted the summary-tf-version-fix branch April 15, 2020 07:12
bileschi pushed a commit to bileschi/tensorboard that referenced this pull request Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants