-
Notifications
You must be signed in to change notification settings - Fork 0
IN 1441 - support a base URL on notebook launch #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Why these changes are being introduced: Based on the interplay of the ALB and the notebook running in an ECS fargate container, we realized that we would need to launch the notebook on a particular URL path. Thankfully, Marimo supports this with a --base-url flag. How this addresses that need: * Adds --base-url / NOTEBOOK_BASE_URL configurations to explicitly set a base url to launch on * Adds flags + env vars to skip a base URL entirely * If neither are passed, the default behavior is to construct a base URL from the repository name + notebook path Side effects of this change: * This aligns with the URL patterns that ALB requests will send to the running notebook. By passing --repo https://github.com/foo/bar and --path super/notebook.py, the resulting base URL will be "bar/super/notebook.py" which is precisely what the ALB will send as requests Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1441
Pull Request Test Coverage Report for Build 17267450290Details
💛 - Coveralls |
Why these changes are being introduced: We realized we could simplify this a bit at the infrastructure level and *always* pass a base URL if needed. This means we can simplify this application to only apply a base URL when explicitly passed. How this addresses that need: * Removes any notion of "skipping" a base URL * CLI arg / env var passes along an explicit base URL, or none is set Side effects of this change: * ALB + ECS service infra is responsible for knowing and setting the base URL Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1441
ehanson8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works as expected, my reviewing-by-commit process led me to an unnecessary question but there's one potential README.md update. Also, I had to use uv run marimo-launcher run rather than just marimo-launcher run since that ran an older version of the code without --base-url, not sure what happened there
| run Launch notebook in 'run' or 'edit' mode. | ||
| ``` | ||
|
|
||
| ### `launcher run` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be marimo-launcher run?
Purpose and background context
This PR adds support for launching a notebook with a URL prefix. It was learned that requests coming from the ALB will likely have a URL request that we cannot control, but it's fairly trivial to just launch the notebook at a specific URL.
Either the CLI arg
--base-urlor the env varNOTEBOOK_BASE_URLwill be set by the ECS service definition.For local devlopment work, all behavior is default absence of a URL prefix; just host + port.
How can a reviewer manually see the effects of these changes?
Run a notebook with a custom prefix:
Output showing running notebook:
Includes new or updated dependencies?
YES
Changes expectations for external applications?
YES
What are the relevant tickets?