Skip to content

Commit 546eafa

Browse files
authored
fix(islands): propagate notebook filename through MarimoIslandGenerator.from_file (#9409)
## Summary Closes #9391 `MarimoIslandGenerator.from_file(path)` previously discarded the source path. `build()` then constructed `AppFileManager` with `filename=None`, the kernel inherited `app_metadata.filename = None`, and `create_main_module(file=None)` fell through to `sys.modules['__main__'].__file__` — i.e. the host process's launcher script. Cells inside notebooks rendered through islands therefore saw `__file__` (and `mo.notebook_dir()`) resolve to the calling script (or to the installed CLI's `.venv/bin/<entry>` shim) rather than to the notebook source. `marimo edit` and `marimo export` were unaffected; only the islands code path was broken. The fix threads the source path through: - `MarimoIslandGenerator.__init__` gains a `_source_filename` field (defaults to `None`, so manual `MarimoIslandGenerator() + add_code()` flows are unchanged). - `from_file()` records the path on the instance. - `build()` passes it to `AppFileManager.from_app(self._app, filename=…)`. - `AppFileManager.from_app` accepts a new optional `filename` kwarg and assigns `_filename` directly. All existing single-arg callers (29 in tests + 1 here) continue to work. `run_app_until_completion` already populates `AppMetadata(filename=file_manager.path, …)`, so once the file manager knows the path the existing kernel / `patch_main_module` plumbing delivers the correct `__file__` — no further changes in `_runtime/`. ### Before / after (issue's exact repro) `/tmp/repro/notebooks/nb.py` prints `__file__` and `mo.notebook_dir()` from a cell. `/tmp/repro/scripts/render.py` calls `MarimoIslandGenerator.from_file(...)`. Before: ``` __file__ = '/tmp/repro/scripts/render.py' notebook_dir() = PosixPath('/tmp/repro/scripts') ``` After: ``` __file__ = '/tmp/repro/notebooks/nb.py' notebook_dir() = PosixPath('/tmp/repro/notebooks') ``` ### Note on backward compatibility This restores the documented / expected semantics, but it **is** a behavior change for any island user who relied on `__file__` resolving to the host process. @mscolnick noted in [#9391](#9391 (comment)) that this may warrant a breaking-change classification — happy to take labeling / release-note guidance. ## Pre-Review Checklist - [x] For large changes, or changes that affect the public API: this change was discussed or approved through an issue — see @mscolnick's comment on #9391: *"this does look like a real issue. we'd gladly accept the contribution to fix this."* - [x] Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it. - [ ] Video or media evidence is provided for any visual changes (optional). — N/A, no visual change. ## Merge Checklist - [x] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [x] Documentation has been updated where applicable, including docstrings for API changes. — `AppFileManager.from_app` docstring updated to document the new `filename` parameter. - [x] Tests have been added for the changes made. — `tests/_islands/test_island_generator.py::test_from_file_propagates_filename_to_cells`. ## Verification - `pytest tests/_islands/` — 11 passed (10 existing + 1 new regression test) - `pytest tests/_server/test_file_manager.py tests/_server/test_sessions.py tests/_server/export/test_exporter.py` — 81 passed (the 15 deselected `test_export_html*` failures are pre-existing on `main` due to no `make fe` artifact and unrelated to this change) - `ruff check` / `ruff format --check` — clean on changed files - `mypy marimo --exclude=marimo/_tutorials/` — same 14 pre-existing errors as `main`; zero new errors introduced
1 parent a338d3c commit 546eafa

4 files changed

Lines changed: 143 additions & 2 deletions

File tree

marimo/_islands/_island_generator.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
import asyncio
55
import json
6+
import os
67
import sys
78
from textwrap import dedent
89
from typing import TYPE_CHECKING, cast
@@ -249,6 +250,10 @@ def __init__(self, app_id: str = "main"):
249250
self._app = InternalApp(App())
250251
self._stubs: list[MarimoIslandStub] = []
251252
self._config = _AppConfig()
253+
# When constructed via ``from_file``, this records the notebook
254+
# source path so cells see ``__file__`` / ``mo.notebook_dir()``
255+
# resolve to the notebook rather than to the host process.
256+
self._source_filename: str | None = None
252257

253258
@staticmethod
254259
def from_file(
@@ -267,6 +272,10 @@ def from_file(
267272
file_manager = load_notebook(filename)
268273

269274
generator = MarimoIslandGenerator()
275+
# Resolve at capture time so a chdir between ``from_file`` and
276+
# ``build`` doesn't change which absolute path cells see for
277+
# ``__file__`` / ``mo.notebook_dir()``.
278+
generator._source_filename = os.path.abspath(filename)
270279
stubs = []
271280
for cell_data in file_manager.app.cell_manager.cell_data():
272281
stubs.append(
@@ -338,7 +347,9 @@ async def build(self) -> App:
338347
raise ValueError("You can only call build() once")
339348

340349
(session, did_error) = await run_app_until_completion(
341-
file_manager=AppFileManager.from_app(self._app),
350+
file_manager=AppFileManager.from_app(
351+
self._app, filename=self._source_filename
352+
),
342353
cli_args={},
343354
argv=None,
344355
)

marimo/_session/notebook/file_manager.py

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,16 +105,28 @@ def filename(self, value: str | Path | None) -> None:
105105
self._filename = _maybe_path(value)
106106

107107
@staticmethod
108-
def from_app(app: InternalApp) -> AppFileManager:
108+
def from_app(
109+
app: InternalApp,
110+
filename: str | Path | None = None,
111+
) -> AppFileManager:
109112
"""Create AppFileManager from an existing InternalApp.
110113
111114
Args:
112115
app: The internal app to wrap
116+
filename: Optional source path for the notebook. When set,
117+
``AppMetadata.filename`` (and therefore ``__file__`` /
118+
``mo.notebook_dir()`` inside cells) resolves to the source
119+
file rather than the host process's ``__main__``.
113120
114121
Returns:
115122
AppFileManager instance
116123
"""
117124
manager = AppFileManager(None)
125+
# Snapshot to an absolute path at assignment time so a later
126+
# ``chdir`` cannot change which file ``self.path`` resolves to.
127+
manager.filename = (
128+
os.path.abspath(str(filename)) if filename is not None else None
129+
)
118130
manager.app = app
119131
return manager
120132

tests/_islands/test_island_generator.py

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,98 @@ def __(mo):
151151
assert "The answer is 42" in stub2.output.data
152152

153153

154+
async def test_from_file_propagates_filename_to_cells(tmp_path: Path):
155+
# Regression test for #9391: cells rendered through
156+
# ``MarimoIslandGenerator.from_file()`` must see ``__file__`` and
157+
# ``mo.notebook_dir()`` resolve to the notebook source, not to the
158+
# host process's ``__main__`` (which under installed CLIs is a
159+
# launcher shim).
160+
marimo_file = tmp_path / "nb.py"
161+
marimo_file.write_text(
162+
"""
163+
import marimo
164+
165+
app = marimo.App()
166+
167+
@app.cell
168+
def __():
169+
import marimo as mo
170+
return (mo,)
171+
172+
@app.cell
173+
def __(mo):
174+
mo.md(f"FILE={__file__} | DIR={mo.notebook_dir()}")
175+
return ()
176+
"""
177+
)
178+
179+
generator = MarimoIslandGenerator.from_file(str(marimo_file))
180+
await generator.build()
181+
182+
captured = generator._stubs[1].output
183+
assert captured is not None
184+
data = captured.data
185+
assert str(marimo_file) in data, (
186+
f"expected __file__ to resolve to {marimo_file}, got: {data}"
187+
)
188+
assert str(marimo_file.parent) in data, (
189+
f"expected notebook_dir() to resolve to {marimo_file.parent}, got: {data}"
190+
)
191+
192+
193+
async def test_from_file_resolves_relative_path_at_capture_time(
194+
tmp_path: Path,
195+
) -> None:
196+
# Companion to ``test_from_file_propagates_filename_to_cells``:
197+
# passing a relative path to ``from_file`` should snapshot the
198+
# absolute path immediately, so a ``chdir`` between ``from_file``
199+
# and ``build`` cannot change which file cells see.
200+
import os
201+
202+
nb_dir = tmp_path / "notebooks"
203+
nb_dir.mkdir()
204+
marimo_file = nb_dir / "nb.py"
205+
marimo_file.write_text(
206+
"""
207+
import marimo
208+
209+
app = marimo.App()
210+
211+
@app.cell
212+
def __():
213+
import marimo as mo
214+
return (mo,)
215+
216+
@app.cell
217+
def __(mo):
218+
mo.md(f"FILE={__file__}")
219+
return ()
220+
"""
221+
)
222+
223+
other_dir = tmp_path / "elsewhere"
224+
other_dir.mkdir()
225+
226+
original_cwd = os.getcwd()
227+
try:
228+
os.chdir(nb_dir)
229+
generator = MarimoIslandGenerator.from_file("nb.py")
230+
# Move to an unrelated directory before ``build`` runs.
231+
os.chdir(other_dir)
232+
await generator.build()
233+
finally:
234+
os.chdir(original_cwd)
235+
236+
captured = generator._stubs[1].output
237+
assert captured is not None
238+
data = captured.data
239+
assert str(marimo_file) in data, (
240+
f"expected __file__ to resolve to {marimo_file}, got: {data}"
241+
)
242+
# And the path shouldn't accidentally resolve under ``other_dir``.
243+
assert str(other_dir / "nb.py") not in data
244+
245+
154246
def test_render_head():
155247
generator = MarimoIslandGenerator()
156248
head_html = generator.render_head()

tests/_server/test_file_manager_filename.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,29 @@ def test_app_file_manager_rename_collision_raises_error(
5959
file_manager.rename(str(target_path))
6060

6161
assert "already exists" in str(exc_info.value.detail)
62+
63+
def test_from_app_snapshots_relative_filename_to_absolute(
64+
self, tmp_path: Path
65+
) -> None:
66+
# ``AppFileManager.from_app(..., filename=...)`` must snapshot a
67+
# relative ``filename`` to an absolute path at call time so a
68+
# later ``chdir`` cannot shift which file ``self.path`` resolves
69+
# to (which is what ``AppMetadata.filename`` / ``__file__`` are
70+
# built from).
71+
import os
72+
73+
from marimo._ast.app import App, InternalApp
74+
75+
nb_dir = tmp_path / "notebooks"
76+
nb_dir.mkdir()
77+
original_cwd = os.getcwd()
78+
try:
79+
os.chdir(nb_dir)
80+
manager = AppFileManager.from_app(
81+
InternalApp(App()), filename="nb.py"
82+
)
83+
expected = str(nb_dir / "nb.py")
84+
os.chdir(tmp_path)
85+
assert manager.path == expected
86+
finally:
87+
os.chdir(original_cwd)

0 commit comments

Comments
 (0)