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

RFC: Improved pip package structure #182

Merged
merged 8 commits into from
Apr 14, 2020

Conversation

annarev
Copy link
Contributor

@annarev annarev commented Nov 27, 2019

Comment period is open until 2019-11-13

Improved pip package structure

Status Proposed
RFC # 182
Author(s) Anna Revinskaya (annarev@google.com)
Sponsor Alex Passos (apassos@tensorflow.org)
Updated 2019-11-27

Objective

We propose to simplify TensorFlow pip package structure to enable IDE features such as autocomplete, jump-to-definition and quick-documentation.

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.

Very happy to see this proposal!

rfcs/20191127-pip-structure.md Outdated Show resolved Hide resolved
rfcs/20191127-pip-structure.md Outdated Show resolved Hide resolved
rfcs/20191127-pip-structure.md Show resolved Hide resolved
@brijk7 brijk7 changed the title Package structure RFC RFC: Improved pip package structure Dec 1, 2019
Copy link

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

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

Very happy to see this proposal as well.

rfcs/20191127-pip-structure.md Show resolved Hide resolved
@martinwicke
Copy link
Member

martinwicke commented Dec 5, 2019 via email

@brijk7
Copy link
Contributor

brijk7 commented Jan 2, 2020

@annarev : is this ready for design review?

@annarev
Copy link
Contributor Author

annarev commented Jan 8, 2020

Yes, it is ready for design review. There are still some things I wanted to test out following comments (unfortunately haven't gotten around to it). But it should be ok to have design review at this point.

@annarev
Copy link
Contributor Author

annarev commented Feb 5, 2020

I think lazy loading is a good option. It is complicated by our current practice of testing which mixes cross-package integration tests with the rest. Otherwise it would not be unclean enough to bother me.

@martinwicke Do you have an example in mind when an integration test would fail because we use lazy-loading as opposed to adding tensorflow_core package?

Would these cases be resolved once estimator just depends on public APIs? I am guessing we can split up the tests if they still cause an issue then.

@martinwicke
Copy link
Member

martinwicke commented Feb 5, 2020 via email

@annarev
Copy link
Contributor Author

annarev commented Feb 5, 2020

Ideally it should be a tree except the lazy-loaded edges. I see some from tensorflow_estimator references in python/tpu and python/estimator directories. My guess is these should be going away and were probably left from before estimator migration.

@annarev annarev closed this Feb 5, 2020
@annarev annarev reopened this Feb 5, 2020
@annarev
Copy link
Contributor Author

annarev commented Feb 5, 2020

Note that from tensorflow import keras issues have been fixed by this PR: tensorflow/tensorflow#34629 from @lgeiger.

Copy link
Member

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

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

Very happy to see this!

rfcs/20191127-pip-structure.md Outdated Show resolved Hide resolved
Co-Authored-By: Gabriel de Marmiesse <gabrieldemarmiesse@gmail.com>
tensorflow-copybara pushed a commit to tensorflow/tensorflow that referenced this pull request Feb 13, 2020
…flow/community#182

PiperOrigin-RevId: 294847967
Change-Id: I327d075a2065e2ccf8ad5317882ebde14e3dc3d6
@mihaimaruseac
Copy link
Contributor

I think this can be merged as it has been already implemented

@bhack
Copy link
Contributor

bhack commented Apr 14, 2020

I think this can be merged as it has been already implemented

@mihaimaruseac Do you meant that the implementation was already merged in the public repo?

@mihaimaruseac
Copy link
Contributor

Yes, the commit with the implementation is literally the latest one on this PR.

1EmKunuzL31

@martinwicke
Copy link
Member

@ematejska can you go through the accepted by unmerged PRs? I think there are a few.

@bhack thanks for pinging these.

@ematejska ematejska added RFC: Accepted RFC Design Document: Accepted by Review and removed RFC: Proposed RFC Design Document labels Apr 14, 2020
@ematejska ematejska merged commit c1d4c1b into tensorflow:master Apr 14, 2020
JimHeo pushed a commit to JimHeo/tensorflow that referenced this pull request Dec 31, 2020
…flow/community#182

PiperOrigin-RevId: 294847967
Change-Id: I327d075a2065e2ccf8ad5317882ebde14e3dc3d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes RFC: Accepted RFC Design Document: Accepted by Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.