Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Jupyter Notebook Extras to automatically load interactive conveniences like explore_dataframe(), %dpip, and %%sparksql when importing the package inside an IPython kernel. It includes configuration options to opt out of these extras, updates dependencies, and adds comprehensive tests. The review feedback highlights a potential memory leak in _ipython.py due to strong references to the IPython shell in _SHELL_STATES, recommending the use of weakref.WeakKeyDictionary. Additionally, a minor grammatical redundancy was pointed out in the README.md documentation.
Automatically initialize interactive notebook extras when importing google.cloud.managed_spark_connect inside an IPython kernel: - Inject colabsqlviz's explore_dataframe() into IPython user_ns - Load the %dpip line magic extension (google.cloud.managed_spark_magics) - Load the %%sparksql cell magic extension (sparksql_magic) - Add google-colabsqlviz>=0.3.0 and sparksql-magic>=0.0.3 to dependencies Add opt-out and runtime configuration controls: - Environment variable: MANAGED_SPARK_CONNECT_ENABLE_EXTRAS=false - IPython traitlet: ManagedSparkConnect.enable_extras = False (supports both persistent file-based config and runtime toggling via %config, tracking and only undoing changes that managed_spark_connect itself performed).
95f3ebd to
62a606f
Compare
| # Imported directly by managed_spark_connect._ipython and | ||
| # managed_spark_magics; previously these only arrived transitively via | ||
| # google-colabsqlviz and sparksql-magic. | ||
| "ipython>=8.0", |
There was a problem hiding this comment.
I feel like we may want to make all of these optional - we have customers that do not use notebooks w/ Spark Connect, for them these dependencies are not necessary.
There was a problem hiding this comment.
The goal is to make the out-of-the box experience as smooth as possible for notebook users.
I agree that we shouldn't introduce performance regressions for non-notebook users. The impact of these changes for non-notebook users are:
- 11M of extra wheel downloads
- 0 runtime cost (short-circuits when get_ipython is not importable or returns None)
I'm sympathetic to wanting to avoid the extra wheels, but there is no perfect option:
pip install google-cloud-spark-connect[notebook]is extra wordy, and agents/humans might fail to add the extra- It's not technically possible to make
pip install google-cloud-spark-connect[no-notebook]remove the dependencies - Optionally importing them only if available means you need to do
pip install google-cloud-spark-connect sparksql-magic google-colabsqlviz [...this list will grow over time...]which is like (1) but worse
On balance, we're saying that the extra wheel download cost is better any of the alternatives 1-3
There was a problem hiding this comment.
I think that it's beyond wheel download, it can cause actual incompatibilities in env, and agents does not need this as well?
I think that to make it practically useful we would need to split it out in the and even exclude pyspark by default:
pip install google-cloud-spark-connect[notebook] pyspark-client
This is still one line step that users/agents will copy from docs, but it allow us to support all the use cases.
There was a problem hiding this comment.
Did google-cloud-spark-connect[notebook] in this PR; requires some additional logic and tests to handle optional imports. Not 100% sure PM has signed off on this but it's ready for review.
There was a problem hiding this comment.
PM approved naming it "interactive" instead of "notebook", updated.
| except Exception: | ||
| pass | ||
|
|
||
| _init_extras() |
There was a problem hiding this comment.
Did we measure latency of this call?
There was a problem hiding this comment.
I can, but it's just loading a couple python modules so I don't imagine it will be more than a few ms.
There was a problem hiding this comment.
We had 20s regression with some AI client libs - I think worth to check this ahead of time.
There was a problem hiding this comment.
Importing is slower than I thought, but still only ~35ms on my laptop. ~27ms of that is importing anywidget. Assuming they are going to run any Spark operations I think this is negligible.
$ t() { MANAGED_SPARK_CONNECT_ENABLE_EXTRAS=0 ipython -c 'import os, time, google.cloud.managed_spark_connect._ipython as m
m._SHELL_STATES.clear(); os.environ["MANAGED_SPARK_CONNECT_ENABLE_EXTRAS"] = "1"
t0 = time.perf_counter(); m._init_extras(); print(f"{(time.perf_counter() - t0) * 1e3:.0f}ms")' 2>/dev/null | tail -1; }
evict() { if [ "$(uname)" = Darwin ]; then sudo purge; else python -c 'import os, site; [os.posix_fadvise(fd := os.open(os.path.join(d, f), os.O_RDONLY), 0, 0, os.POSIX_FADV_DONTNEED) or os.close(fd) for p in site.getsitepackages() for d, _, fs in os.walk(p) for f in fs]'; fi; }
for i in 1 2 3; do echo "hot: $(t) cold page cache: $(evict; t) no .pyc: $(PYTHONPYCACHEPREFIX=$(mktemp -d) t)"; done
MANAGED_SPARK_CONNECT_ENABLE_EXTRAS=0 python -X importtime -m IPython -c 'import os, sys, google.cloud.managed_spark_connect._ipython as m
m._SHELL_STATES.clear(); os.environ["MANAGED_SPARK_CONNECT_ENABLE_EXTRAS"] = "1"; print("MARK", file=sys.stderr, flush=True); m._init_extras()' 2>&1 >/dev/null | awk -F'|' 'f && $2 >= 5000 {l[n++] = sprintf("%6.1fms %s", $2 / 1000, $3)} /^MARK$/ {f = 1} END {print "cumulative import time during _init_extras (>=5ms):"; while (n) print l[--n]}'
hot: 47ms cold page cache: 56ms no .pyc: 98ms
hot: 36ms cold page cache: 45ms no .pyc: 102ms
hot: 35ms cold page cache: 47ms no .pyc: 102ms
cumulative import time during _init_extras (>=5ms):
33.6ms google.colabsqlviz.explore_dataframe
31.6ms google.colabsqlviz.interactive_viz
27.6ms anywidget
20.2ms anywidget.widget
19.8ms ipywidgets
19.4ms ipywidgets.widgets
6.8ms anywidget._traits
6.7ms anywidget._descriptor
6.0ms anywidget._file_contents
5.8ms psygnal
google-colabsqlviz, ipython, sparksql-magic and traitlets are now only installed with google-cloud-spark-connect[interactive], so that users who don't use notebooks don't pay for them. For consistency, the feature is now called "interactive extras" rather than "notebook extras" throughout. At runtime, each extra is initialized only if its dependency is installed; one that isn't installed is skipped silently, and only installed-but-broken ones produce the "Failed to load interactive extras" message. The package itself only imports _ipython when traitlets is available. Since the set of extras now depends on what happens to be installed, replace the explore_dataframe()-specific message with a single line listing every extra that was loaded, e.g.: Loaded interactive extras: explore_dataframe(), %dpip, %%sparksql. To disable: %config ManagedSparkConnect.enable_extras = False Extras that were already present (e.g. loaded by the user) are not listed.
Automatically initialize interactive notebook extras when importing google.cloud.managed_spark_connect inside an IPython kernel:
Add opt-out and runtime configuration controls:
Dependency & Footprint Justification
The new hard dependencies added by this PR (
google-colabsqlviz,sparksql-magic) add just a few MiB of wheel downloads. In more detail:All core data dependencies (
pyspark,pandas>=2.0.0,pyarrow>=10.0.1,protobuf>=4.24.0,packaging>=20.0) are already satisfied bypyspark[connect]andgoogle-api-core. Installing both libraries causes 0 upgrades or downgrades to existing packages.sparksql-magicHas Zero Extra Transitive Cost:sparksql-magic(4.2 KiBwheel,6.9 KiBextracted,14.4 KiBwith.pyc) only depends onpysparkandipython—a strict subset ofgoogle-colabsqlvizandipykernel. Adding it alongsidegoogle-colabsqlvizadds 0 additional transitive dependencies.In local notebook kernels (e.g., VS Code or Jupyter, which require
ipykernel), the entireipythonstack is already present. Adding both libraries requires downloading just 3.59 MiB of wheels (~1.7% of the existing install size dominated bypyspark@ ~435 MiB wheel andpyarrow@ ~130 MiB):.pyc, e.g.uv).pyc(pip)pip)ipykernel+ipywidgets)google-colabsqlviz,sparksql-magic,anywidget,psygnal)ipykernelonly)ipywidgets,jupyterlab-widgets,widgetsnbextension)ipythonstack)(Note: Pure Python code across all 7 packages added in the minimal VS Code kernel scenario is < 1 MiB. The remaining extracted space is static frontend JS bundles/source maps in
widgetsnbextensionandanywidget[~11.5 MiB],psygnal's compiled mypyc.sobinary [~1.3 MiB], andpip's.pycbytecode cache duplicating embedded JS strings incolabsqlviz[~1.0 MiB].)