Repository navigation
Conversation
Add a reusable, standard-library bootstrap utility in scripts/notebook-setup/ for tutorials and sample notebooks that depend on sibling Python modules when opened standalone in Google Colab or Colab Enterprise. - Sparse-clones only the target sample subdirectory into a managed cache folder, self-heals interrupted non-git directories, fetches and checks out FETCH_HEAD on persistent runtimes, installs requirements.txt, and reloads imported sibling sample modules. - Preserves existing local developer and CI repository checkouts without running git fetch or git checkout -f. - Includes unit tests in scripts/notebook-setup/tests/notebook_setup_test.py.
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable notebook setup helper (notebook_setup.py) along with tests and documentation to bootstrap sample environments in Google Colab or local checkouts. The review feedback highlights several improvement opportunities: handling potential None values in sys.modules to prevent runtime crashes, safely handling cases where the repository root path is a file rather than a directory, removing the --upgrade flag from pip install to avoid breaking pre-installed Colab packages, and using a more robust parent-directory search in the README usage example.
| for mod_name in sorted(local_stems): | ||
| if mod_name in sys.modules: | ||
| importlib.reload(sys.modules[mod_name]) |
There was a problem hiding this comment.
In Python, sys.modules can contain None values (e.g., to disable imports or represent failed relative imports). Attempting to call importlib.reload(None) will raise a TypeError: reload() argument must be a module.
To prevent potential runtime crashes, guard the reload by checking if the module is not None. Additionally, using .get() avoids double lookup in sys.modules.
| for mod_name in sorted(local_stems): | |
| if mod_name in sys.modules: | |
| importlib.reload(sys.modules[mod_name]) | |
| for mod_name in sorted(local_stems): | |
| module = sys.modules.get(mod_name) | |
| if module is not None: | |
| importlib.reload(module) |
| if repo_root.exists() and not (repo_root / ".git").exists(): | ||
| shutil.rmtree(repo_root) |
There was a problem hiding this comment.
If repo_root exists but is a file (for example, if a file was accidentally created with the same name as the expected clone directory), shutil.rmtree will raise a NotADirectoryError.
Using is_dir() to check if it is a directory before calling shutil.rmtree, and falling back to unlink() if it is a file, makes the cleanup process much more robust.
if repo_root.exists() and not (repo_root / ".git").exists():
if repo_root.is_dir():
shutil.rmtree(repo_root)
else:
repo_root.unlink()| if install_requirements and req_file.is_file(): | ||
| subprocess.run( | ||
| [ | ||
| sys.executable, | ||
| "-m", | ||
| "pip", | ||
| "install", | ||
| "--upgrade", | ||
| "--quiet", | ||
| "-r", | ||
| str(req_file), | ||
| ], | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
Using the --upgrade flag when installing requirements in a notebook environment (such as Google Colab) can be highly inefficient and risky. It forces pip to upgrade all dependencies (and their dependencies) to the latest versions, which can take a long time, consume significant bandwidth, and potentially break pre-installed packages in the Colab environment due to version conflicts.
Removing --upgrade allows pip to only install missing packages or packages that do not satisfy the version constraints, which is much faster and more stable.
if install_requirements and req_file.is_file():
subprocess.run(
[
sys.executable,
"-m",
"pip",
"install",
"--quiet",
"-r",
str(req_file),
],
check=True,
)| for candidate in ( | ||
| Path("../../scripts/notebook-setup/notebook_setup.py"), | ||
| Path("scripts/notebook-setup/notebook_setup.py"), | ||
| ): | ||
| if candidate.is_file(): | ||
| SETUP_PATH = candidate | ||
| break |
There was a problem hiding this comment.
The hardcoded relative paths ../../scripts/... and scripts/... will fail to find the local script if the notebook is nested deeper in the repository (for example, the people-and-planet-ai/weather-forecasting/notebooks/1-overview.ipynb notebook is 3 levels deep, so it would require ../../../). This would force the notebook to always download the script from GitHub even during local development.
Searching upward through parent directories makes the local script detection extremely robust regardless of the notebook's nesting depth.
| for candidate in ( | |
| Path("../../scripts/notebook-setup/notebook_setup.py"), | |
| Path("scripts/notebook-setup/notebook_setup.py"), | |
| ): | |
| if candidate.is_file(): | |
| SETUP_PATH = candidate | |
| break | |
| for parent in (Path.cwd(), *Path.cwd().parents): | |
| candidate = parent / "scripts/notebook-setup/notebook_setup.py" | |
| if candidate.is_file(): | |
| SETUP_PATH = candidate | |
| break |
Summary
Adds a reusable standard-library bootstrap helper (
scripts/notebook-setup/notebook_setup.py) so tutorial notebooks acrosspython-docs-samplesdo not have to inline ~65 lines ofgit sparse-checkout,pip install, andimportlib.reloadplumbing at the top of each.ipynbfile.Key Behaviors
.ipynbruns: Creates or updates a managed sparse checkout (samples-<name>), self-heals partial non-git directories, fetches and checks out the latestFETCH_HEADfor the requested branch, installsrequirements.txt, and reloads any previously imported sibling modules.git fetchorgit checkout -f), switching into the sample directory and ensuringsys.pathis configured.scripts/notebook-setup/tests/notebook_setup_test.pywith target-safemock.patch.object(..., autospec=True).