fix(reload): exclude marimo package from modulefinder crawl#10249
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA |
|
Thanks! Can you clean up the comments to be more general? Aside from that LGTM |
|
Thanks for the review! Cleaned up the comments to be more general — dropped the issue-specific detail and namespace-package explanation, kept the docstring to a one-liner and trimmed the root-package note. Ready for another look. |
There was a problem hiding this comment.
Pull request overview
This PR addresses autoreload dependency discovery failing for editable installs when modulefinder.ModuleFinder encounters marimo’s namespace-package layout, by ensuring marimo modules are excluded from the dependency crawl (issue #10225).
Changes:
- Add
_is_marimo_internal_module_name()to identifymarimo/marimo.*module names. - Update
_get_excluded_modules()to exclude marimo internal modules (in addition tosite-packagesmodules). - Add unit tests covering marimo exclusion behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
marimo/_runtime/reload/module_watcher.py |
Adds marimo-internal module detection and expands excluded-module computation to prevent modulefinder crashes on marimo’s package tree. |
tests/_runtime/reload/test_module_watcher.py |
Adds tests validating marimo exclusion behavior for editable-install scenarios. |
Comments suppressed due to low confidence (1)
marimo/_runtime/reload/module_watcher.py:127
_get_excluded_modules()now unconditionally adds "marimo", which makes the function return a non-empty set even when the providedmodulesdict contains only user modules (this breaks the existingtest_get_excluded_modules_emptyexpectation and changes the semantics from "exclude third-party/marimo modules present" to "always exclude marimo"). If the intent is only to avoid crawling marimo when it's actually loaded (the production call passessys.modules, where marimo will be present), gate this addition on whether any marimo module names are present inmodules.
sys_modules: dict[str, types.ModuleType],
) -> dict[str, types.ModuleType]:
"""Returns the set of modules used by the graph that have been modified"""
stdlib modulefinder crashes on marimo namespace packages (dirs without __init__.py). Editable installs are not under site-packages, so marimo was never excluded and autoreload silently lost dependency chains. Always exclude marimo and marimo.* module names from the finder excludes set, and update the local-only unit expectation accordingly. Closes marimo-team#10225. Signed-off-by: Bartok9 <danielrpike9@gmail.com>
abe02f1 to
2d89acf
Compare
|
Thanks @mscolnick — and sorry the approval got dismissed. CI red was real: |
|
I have read the CLA Document and I hereby sign the CLA |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.15-dev63 |
This pull request was authored by a coding agent.
AI-assisted; human-reviewed line-by-line. Submitted under Daniel's standing Top10 merge-culture campaign instruction for Bartok9.
Summary
Closes #10225.
modulefinder.ModuleFindercannot walk namespace packages. marimo ships several (marimo/_dependencies, etc.). Pip installs hide this because marimo lives undersite-packagesand is already excluded as third-party; editable installs do not, so dependency analysis fails quietly and autoreload misses chains.Change
_is_marimo_internal_module_nameformarimo/marimo.*_get_excluded_modulesalways adds"marimo"and filters internal namesTests
pytest tests/_runtime/reload/test_module_watcher.py::TestGetExcludedModulesPre-Review Checklist
Merge Checklist