Skip to content

Add checks for code quality: usage of tf.compat.v1 and experimental APIs #1544

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

Merged
merged 2 commits into from
Apr 7, 2020
Merged

Add checks for code quality: usage of tf.compat.v1 and experimental APIs #1544

merged 2 commits into from
Apr 7, 2020

Conversation

gabrieldemarmiesse
Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse commented Apr 2, 2020

This should save us some work during code reviews.

We might want to discuss for this policy about experimental APIs though. See tensorflow/community#218
Should we be more permissive? Less permissive?

Closes #1279

@boring-cyborg boring-cyborg bot added the test-cases Related to Addons tests label Apr 2, 2020
@gabrieldemarmiesse gabrieldemarmiesse changed the title Add checks for code quality: tf.compat.v1 and experimental APIs Add checks for code quality: usage of tf.compat.v1 and experimental APIs Apr 2, 2020
@AakashKumarNain
Copy link
Member

IMO we should definitely block anything that uses tf.compat.v1. If we allow this, we would be in somewhat similar scenario as py2 -> py3.

Regarding experiemtal, although most of the time things remain the same when they come out of experimental policy, IMO we should still defer its usage.

@gabrieldemarmiesse
Copy link
Member Author

gabrieldemarmiesse commented Apr 6, 2020

The reason behing disallowing experimental in user-facing code is to be forward compatible. In a perfect world, since the public API of TF is backward compatible (with the exception of experimental APIs) if we use only the public API, the users should be able to use any version of TF above the recommended one and it will work.

Experimental API are doomed to have their name changed as some points, which means that when they do and we use those APIs, it forces users to update their TF addons version. Users should have to think as little as possible about compatibility between packages.

@AakashKumarNain
Copy link
Member

I am fine with the changes. @seanpmorgan thoughts?

Copy link
Member

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Yeah I agree with this goal. It may be possible we're forced to use an experimental API, but this PR still enables a workaround with the blacklist if there are no other options.

@seanpmorgan seanpmorgan merged commit 767efc1 into tensorflow:master Apr 7, 2020
jrruijli pushed a commit to jrruijli/addons that referenced this pull request Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes test-cases Related to Addons tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests to ensure that we don't use experimental/private/deprecated tensorflow APIs
4 participants