refactor(omnidreams/webrtc): serve packaged web assets via importlib.resources#343
Conversation
…resources Discover the packaged omnidreams.webrtc web/ directory through importlib.resources (files + as_file) inside an ExitStack instead of deriving it from Path(__file__), and close the stack on app cleanup. This keeps the static asset directory resolvable in non-standard installs (zip-imports, editable wheels, relocated site-packages), following the canonical pattern from the issue. The optional repo-root /assets/ mount stays backward-compatible: it is still mounted when the directory exists (checkout case) and skipped otherwise, so wheel-installed deployments no longer depend on a repo root four parents up. Refs NVIDIA#329 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| resource_stack = ExitStack() | ||
| web_dir = resource_stack.enter_context(as_file(WEB_DIR_RESOURCE)) | ||
|
|
||
| app = create_webrtc_app( | ||
| web_dir=WEB_DIR, | ||
| web_dir=web_dir, | ||
| session_manager=manager, | ||
| preload_name="Omnidreams", | ||
| request_session_url=request_session_url, | ||
| ) | ||
| app["package_resource_stack"] = resource_stack | ||
| app.on_cleanup.append(_close_package_resources) |
There was a problem hiding this comment.
create_webrtc_app raises
resource_stack is entered (and potentially extracts a temp dir via as_file) before create_webrtc_app is called. If create_webrtc_app raises — or if the subsequent app["package_resource_stack"] = resource_stack line ever fails — the stack is never closed and the temporary directory is orphaned. The cleanup handler is only registered after everything succeeds, so the exception path has no teardown at all.
The defensive fix is to guard the setup block: wrap lines 132–142 in a try/except that calls resource_stack.close() then re-raises, so the stack is always closed on failure.
| def test_create_app_keeps_package_web_resource_materialized() -> None: | ||
| app = create_app( | ||
| session_manager=FakeSessionManager(), | ||
| request_session_url="http://127.0.0.1:8080/request_session", | ||
| ) | ||
| try: | ||
| assert isinstance(app["package_resource_stack"], ExitStack) | ||
| assert _close_package_resources in app.on_cleanup | ||
|
|
||
| static_resources = [ | ||
| resource | ||
| for resource in app.router.resources() | ||
| if getattr(resource, "canonical", "") == "/static" | ||
| or resource.get_info().get("prefix") in {"/static", "/static/"} | ||
| ] | ||
| assert len(static_resources) == 1 | ||
| web_dir = static_resources[0].get_info()["directory"] | ||
| assert web_dir.is_dir() | ||
| assert "Omnidreams WebRTC Drive" in ( | ||
| web_dir / "request_session.html" | ||
| ).read_text() | ||
| finally: | ||
| app["package_resource_stack"].close() |
There was a problem hiding this comment.
ci_gpu marker unnecessarily
pytestmark = pytest.mark.ci_gpu at module level marks every test in the file. The two new tests (test_create_app_keeps_package_web_resource_materialized and test_create_app_skips_absent_repo_assets_mount) never touch a GPU — they only verify app construction, route registration, and ExitStack wiring. Running them only on GPU runners means they're skipped in all standard CI environments, which is exactly where pure unit tests should run earliest. Consider overriding the marker or moving these two tests to a separate CPU-only test module.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
What
Refactors the Omnidreams WebRTC server to discover its packaged
web/directory throughimportlib.resourcesinstead ofPath(__file__), following the canonical pattern from #329.server.pynow:files("omnidreams.webrtc").joinpath("web")and materializes them viaas_file(...)inside anExitStack.app["package_resource_stack"]) and closes it from anon_cleanuphandler, so the directory stays valid for the app's entire lifetime and is cleaned up on shutdown.create_appsignature and the public route surface unchanged.Why
As called out in #329, deriving packaged assets from
Path(__file__).resolve()can break in non-standard installs (zip-imports, editable wheels, relocated site-packages). Theimportlib.resourcespath resolves correctly regardless of how the package is laid out on disk, and theExitStack/as_filewrapping is the defensive layer the issue describes for cases where the resource is not already a real filesystem path.Backward-compatible
/assets/mountThe optional repo-root
REPO_ASSETS_DIR = Path(__file__).resolve().parents[4] / "assets"mount is left intact and remains guarded byif REPO_ASSETS_DIR.is_dir():. In a checkout it still mounts/assets/; in a wheel-installed deployment where the repo root is absent, the mount is simply skipped rather than raising. A short comment notes this.Tests
Two focused tests added to
test_webrtc_server_routes.py:test_create_app_keeps_package_web_resource_materialized— asserts theExitStackis registered on the app, the_close_package_resourcescleanup is wired intoon_cleanup, and the materializedweb/directory is real and readable.test_create_app_skips_absent_repo_assets_mount— monkeypatchesREPO_ASSETS_DIRto a missing path (installed-wheel layout) and assertscreate_appstill constructs and registers no/assets/route.The existing route assertions are unchanged and continue to exercise the new path automatically through the aiohttp
TestServer/TestClientlifecycle.Scope
This PR covers the omnidreams
webrtc/server.pycase the issue centers on (the closest sibling to #328). The other__file__sites listed in the issue (lingbot, hy_worldplay, flashvsr, interactive_drive CLI, ludus_renderer JIT) each need their own resource-bundling decisions and are intentionally left out of this change.AI was used for assistance.
Refs #329