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

init: fix top-level module reloading #2005

Merged
merged 6 commits into from
Mar 13, 2019
Merged

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Mar 13, 2019

Summary:
Fixes #1989 (also described in a new comment in __init__.py).

This will require a Google-internal sync change: http://cl/238144308.

Test Plan:
Regression test added. It fails before this change in Python 2 and 3,
and passes after this change in both as well. The http://cl/238144308
test plan also passes.

wchargin-branch: reload-tensorboard

Summary:
Fixes #1989 (also described in a new comment in `__init__.py`).

This will require a Google-internal sync change, which I’ll link in
shortly.

Test Plan:
Regression test added. It fails before this change in Python 2 and 3,
and passes after this change in both as well.

wchargin-branch: reload-tensorboard
wchargin-branch: reload-tensorboard
(Just for consistency with the rest of the module—we’re not so far down
the rabbit hole that this is a functional change.)

wchargin-branch: reload-tensorboard
@wchargin wchargin requested a review from nfelt March 13, 2019 00:59
@wchargin wchargin marked this pull request as ready for review March 13, 2019 00:59
wchargin-branch: reload-tensorboard
tensorboard/lib_test.py Outdated Show resolved Hide resolved
wchargin-branch: reload-tensorboard
wchargin-branch: reload-tensorboard
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

FWIW, one other potential way to address this would be for the lazy loading logic itself to check if the named module already exists in sys.modules, and if it does exist, just return that module immediately. This would mean that after reloading, the modules would be pre-resolved rather than lazy, so it's different from the approach here, but also seems correct. Thoughts?

@wchargin
Copy link
Contributor Author

check if the named module already exists in sys.modules

Presuming that you mean the argument to lazy.lazy_load… this seems
like an opportunity for confusion/surprise. These names aren’t required
to correspond to actual modules at all: e.g., the tensorboard.compat
LazyModules have module-like names, but no such modules exist. For this
string to sometimes be used decoratively and sometimes be used
semantically would be an unnecessary inconsistency, imo. If it were to
always behave semantically, then that would be fine—but as of #1781
that is deliberately not the case.

@nfelt
Copy link
Contributor

nfelt commented Mar 13, 2019

Fair enough. I think it is fair to require that the names passed to the decorator match the actual importable name of the decorated symbol, which is all that's really required for this to work (if there is no module in sys.modules, you fall back to the lazy function, regardless of whether there ever is such a module). But what you've done is fine too.

@wchargin
Copy link
Contributor Author

if there is no module in sys.modules, you fall back to the lazy
function, regardless of whether there ever is such a module

Yep, understood. It only really breaks if you have a lazy_load("foo")
and a module foo but the lazy module loads a different module, which
seems like a bad idea all around.

I think it is fair to require that the names passed to the decorator
match the actual importable name of the decorated symbol, which is all
that's really required for this to work

This already doesn’t hold within Google, though. :-)

@wchargin wchargin merged commit e53958f into master Mar 13, 2019
@wchargin wchargin deleted the wchargin-reload-tensorboard branch March 13, 2019 02:01
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.

2 participants