FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Monkeypatch bigquery and gcs at module load time to avoid always load… by vimota · Pull Request #590 · Kaggle/docker-python · GitHub

Monkeypatch bigquery and gcs at module load time to avoid always load… - #590

Merged
vimota merged 4 commits into
masterfrom
modules
Jul 18, 2019
Merged

vimota merged 4 commits into
masterfrom
modules

Conversation

vimota commented Jul 18, 2019

Copy link
Copy Markdown
Contributor

…ing bigquery/gcs and all its dependencies.

I realize this change is pretty confusing and took me a while to get to a working state, so feel free to comment, ask for clarifications. I'll try to explain what it's doing but let me know if it's not clear.

This tries to use import hooks (as suggested by Vincent, thanks!) to do the monkeypatching at module load time (versus when sitecustomize is loaded prior to user code execution). This resolves the issue of having tons of modules (google.cloud.* and all its deps) loaded for Kernels that may never use bigquery/gcs. The strategy is: setup an import hook that checks when one of our special libraries is being loaded and instead of loading the one in the module path, load our monkeypatched one.

The issue is, our monkeypatching uses kaggle_gcp which itself loads google.cloud.bigquery (it has to, since we do the monkeypatching by subclassing bigquery), which causes a circular dependency/recursive imports. We get around this by adding a check in the module finder hook to return the actual bigquery library if the call stack originates from kaggle_gcp, thus avoiding the circular dependency. We still have the issue that if the user (or test code) calls import kaggle_gcp before from google.cloud import bigquery, we will load the true bigquery library during the kaggle_gcp load and never end up monkeypatching it since google.cloud.bigquery will already have been loaded. To solve this, I add a init() to the end of kaggle_gcp so if ever kaggle_gcp is imported, then the monkeypatching will guaranteed to be done even after the true bigquery library has been loaded.

vimota requested review from harrisse, ifigotin and rosbo July 18, 2019 06:07

vimota commented Jul 18, 2019

Copy link
Copy Markdown
Contributor Author

vimota commented Jul 18, 2019

Copy link
Copy Markdown
Contributor Author

Really good reference on import hooks here https://github.com/0cjs/sedoc/blob/master/lang/python/importers.md

rosbo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks Victor for diving into the weeds of Python import system and find this solution for b/137572393! :)

Our users will really appreciate.

Comment thread patches/sitecustomize.py Outdated

ifigotin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM (very cool that you found a solution here!!!)

fyi: we may have a tiny merge conflict in sitecustomize.py due to my pending PR.

Comment thread patches/sitecustomize.py Outdated
Comment thread patches/sitecustomize.py Outdated
# since we call kaggle_gcp to load the module.
if self._is_called_from_kaggle_gcp():
return None
spec = importlib.machinery.ModuleSpec(fullname, GcpModuleLoader())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: why not just return importlib.machinery.ModuleSpec(fullname, GcpModuleLoader()), is it more Pythonic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done.

Was leftover from when I was logging the spec :)

ifigotin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM great!

vimota commented Jul 18, 2019

Copy link
Copy Markdown
Contributor Author

Tests pass, and I've checked that starting python in the container indeed doesn't load all of google.cloud.* like it used to. Now I'm building the container locally to test on a Kernel just to make sure everything runs correctly.

vimota merged commit db3252e into master Jul 18, 2019
vimota deleted the modules branch July 18, 2019 22:11

vimota commented Jul 18, 2019

Copy link
Copy Markdown
Contributor Author

Tested it end to end locally, and looks good! Would still be good to test more thoroughly when we have a staging image to release by running with existing bigquery kernels in prod.

This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL