Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
1 issue found across 6 files
Architecture diagram
sequenceDiagram
participant User as User Code
participant Visitor as ScopedVisitor
participant Compiler as compile_cell()
participant Cache as Serialize/Dequeue (hash.py)
participant Runtime as Kernel Runtime
Note over User,Runtime: Private Import Resolution Flow
User->>Visitor: Parse cell with underscore import (e.g., "import _private.module" or "from foo import _bar")
Visitor->>Visitor: _get_alias_name() - Skip mangling for all import aliases
alt Import uses "as _name" pattern
Visitor->>Visitor: Leave asname unmangled (cell-local via is_local check in compile_cell())
else Standard underscore import
Visitor->>Visitor: Leave basename unmangled
end
Note over Visitor: visit_Name() - Handle underscore refs
alt Load reference to underscore name
Visitor->>Visitor: Check if block defines unmangled name
opt Unmangled name defined (e.g., by import)
Visitor->>Visitor: Keep node.id as original (no mangling)
end
opt Only mangled name defined
alt Nested scope OR undefined
Visitor->>Visitor: Apply mangling
end
end
end
Visitor-->>Compiler: defs contains import names (incl. underscore-prefixed)
Compiler->>Compiler: Build nonlocals set
alt Name starts with _
alt Import definition present
Compiler->>Compiler: Include in nonlocals (graph node)
else No import
Compiler->>Compiler: Include in temporaries (cell-local)
end
end
User->>Cache: Cache/Serialize cell with underscore import reference
Cache->>Cache: serialize_and_dequeue_content_refs()
Cache->>Cache: Mangle local_ref for lookup
alt Mangied key not in imports
Cache->>Cache: Try unmangled key in imports
alt Unmangled key found (import case)
Cache->>Cache: Use unmangled key for import resolution
end
end
User->>Runtime: Execute cells
Runtime->>Runtime: test_underscore_prefixed_import_in_cell()
Note over Runtime: Same cell: underscore import resolves correctly
Runtime->>Runtime: test_underscore_prefixed_import_across_cells()
Note over Runtime: Cross-cell: underscore import works at runtime via shared globals
Note over Runtime: NO reactive graph edge created (trade-off)
Runtime->>Runtime: test_lru_cache_underscore_alias()
Note over Runtime: Cache hits work correctly with unmangled import references
alt Error case
Runtime->>Runtime: test_underscore_imports_never_mangled()
Note over Runtime: No "_cell_test_" suffix in unparsed AST
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… retain cache-hash fix Reverts the visitor/compiler changes that promoted underscore-prefixed 'as' imports to global graph defs. Two cells each doing 'import numpy as _np' no longer raise MultipleDefinitionError, restoring the documented contract that underscore-prefixed names are cell-local/private. Keeps the hash.py fix: a cached function referencing a mangled underscore import (e.g. '@_private.lru_cache(...)') can surface the raw unmangled name as a ref, so the import lookup must accept either the mangled ref or the raw local_ref. Without this, cache hashing raises 'cannot pickle module object'.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="marimo/_ast/compiler.py">
<violation number="1">
P1: Private-prefixed import definitions are excluded from cell defs, breaking cross-cell import resolution for `import _private...` cases. This regresses the import edge case this PR is intended to fix.</violation>
</file>
<file name="marimo/_ast/visitor.py">
<violation number="1">
P1: Nested-scope loads of underscore imports are over-mangled. This can rewrite valid references (e.g. `import _pkg.mod` then use `_pkg` inside a function) to an undefined mangled name.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where underscore-prefixed import aliases (e.g. import marimo as _private) could break cache hashing by causing module imports to be misclassified and incorrectly serialized/pickled. It also adds regression tests to ensure underscore-prefixed import aliases remain cell-local (mangled) while still resolving correctly within the same cell and across cells.
Changes:
- Update cache hashing to recognize imports keyed by either the mangled name or the raw (unmangled) local ref.
- Add cache regression tests covering
_-prefixed import aliases withcacheandlru_cache. - Add AST visitor and runtime tests validating underscore-prefixed import alias mangling and resolution behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
marimo/_save/hash.py |
Accept either mangled or raw import keys when detecting module imports during content hashing to avoid pickling module objects. |
tests/_save/test_cache.py |
Adds regression tests ensuring caching works when marimo is imported as an underscore-prefixed alias. |
tests/_runtime/test_runtime.py |
Adds runtime regression tests verifying underscore-prefixed import aliases resolve within-cell and don’t conflict across cells. |
tests/_ast/test_visitor.py |
Adds AST visitor test asserting underscore-prefixed as imports are mangled to cell-local names and remain resolvable. |
| # Regression: `import marimo as _private` (underscore alias) used to | ||
| # blow up cache hashing because mangled scope keys (`_cell_<id>__private`) | ||
| # don't match the unmangled-ref lookup used during the cache attempt. |
| # The alias is cell-local, so it never leaks into globals. | ||
| assert "_mo" not in k.globals |
|
This pull request has been automatically marked as stale because it has not had activity in 30 days. It will be closed in 14 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open. Thank you for your contribution! |
…refs - visitor: a local reference stays raw only when the raw name is a top-level definition (only no-alias underscore imports produce one); everything else mangles, including names the cell never defines, so one cell's raw import can no longer leak into another through globals - executor: demangle NameError messages so script mode and tracebacks show the name the user wrote - cell_runner: hint on NameErrors for cell-local names defined in another cell, suggesting a re-aliased import where applicable - docs: document the underscore-import carve-out in reactivity.md
# Conflicts: # marimo/_save/hash.py
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.15-dev67 |
📝 Summary
Replaces #9308 and #9309. Closes #9151, closes #10223.
A scoping change introduced in #8762 (0.22.0) made
_-prefixed imports private. The intent is consistent with marimo's private-variable pattern, but three edge cases were mishandled:from pkg import _camera) were mangled inside nested scopes (function bodies, decorators, class bases), producingNameError: name '_cell_<id>_camera' is not defined(import of internal modules is not working any more >0.21.1 #9151). These imports are deliberately defined under their raw name — the user has no control over the upstream symbol — so their references must stay raw everywhere.import marimo as _mo+@_mo.lru_cache→cannot pickle 'module' object).Intended behavior
import foofoo, publicimport foo as _f/import a.b as _f_f, cell-localfrom a.b import _c_c, cell-localimport _private.submodule_private, cell-localfrom ibis import __, cell-localfrom ibis import _ as lib(#10223)_x = ..._x, cell-localCell-local imports never trigger
MultipleDefinitionError: two cells may bothimport numpy as _nporfrom pkg import _c. Mangling imports wholesale (#9309's approach) was ruled out becauseimport _private.submodulehas no resolvable local name to mangle.