-
Notifications
You must be signed in to change notification settings - Fork 0
IN 1415 - first pass at CLI #10
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:
This builds on the project scaffolding to provide a CLI capable
of preparing the notebook environment and running the notebook.
This does NOT yet contain much of any meaningful testing.
How this addresses that need:
* Establishes CLI command and signature
* Supports a pre-existing ("mounted") notebook location or
a git clone for a repository
* Prepares a full notebook path to run
* Prepares a marimo subprocess command that handles
inlined or external dependencies
* Whether inline or external dependencies, the process
is always isolated via `uv` given the `--sandbox` or
`--with-requirements` flags.
Side effects of this change:
* None
Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/IN-1415
Pull Request Test Coverage Report for Build 17051383608Details
💛 - Coveralls |
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.
Good stuff, a few comments!
Why these changes are being introduced: With the --no-token or --token flag, it was a bit confusing and it did not allow for setting an explicit token for the noteobok. We need to support setting the token such that we can know it advance. How this addresses that need: * Changes the CLI arg to `--token:str|None` which when None sets `--no-token` in marimo launch else sets the explicit token passed Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1415
How this addresses that need: * More explicit names for helper functions around establishing notebook paths and cloning repositories * Support checkout of a git branch on clone Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-1415
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.
Great work!
| # set token if passed | ||
| if token: | ||
| cmd += ["--token", "--token-password", token] | ||
| else: |
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.
Much cleaner, great work!
| envvar="NOTEBOOK_REPOSITORY", | ||
| help="git repository URL containing the notebook (env: NOTEBOOK_REPOSITORY)", | ||
| ) | ||
| @click.option( |
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.
Good addition!
| if mount: | ||
| p = Path(mount) | ||
| if not p.exists(): | ||
| notebook_dir_path = Path(mount) |
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.
Always appreciate explicit var names!
| notebook_dir_path = workdir / f"notebook-clone-{uuid.uuid4()}" | ||
|
|
||
| clone_notebook_repository(notebook_dir_path, repo, repo_branch) |
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.
Beautiful!
Purpose and background context
This PR is the first pass at a CLI capable of preparing a Marimo notebook for running, then launching it either in
runoreditmode for interaction. In this way, this CLI will be running as long as the notebook is. Perhaps even more than a "launcher" it's a "wrapper" of sorts.Apologies for a larger PR. This felt very difficult to chunk, given how much it changed from moment to moment as the pieces were falling into place. For review, it's recommend to focus on these chunks:
Overall flow of this CLI:
launcher runwith argumentsNOTEBOOK_REPOSITORYenv var where it goes out and clones a repositorynotebook.pyin the root of the repository, or you can provide runtime arguments indicating where that is2718(again, configurable)Something that may not be immediately obvious: there is no ECS task/service associated with this app / ECR image! This will take a little getting used to, but think of this application as a generic notebook launcher. When we do finally define an ECS task/service, it will oriented around an actual notebook repository (e.g.https://github.com/MITLibraries/marimo-helloworld). Invoking that task/service will run this CLI / ECR image, but passing
NOTEBOOK_REPOSITORYenv var to this CLI to inform it where to clone the repo from.As such, you'll notice in testing below we interact with this more like a true Docker image than an ECS task/service we can run.
How can a reviewer manually see the effects of these changes?
Run a tests fixture notebook with inline dependencies
1- Run the following Makefile command:
Note a few things here:
['uv', 'run', 'marimo', 'run', '--headless', '--host', '0.0.0.0', '--port', '2718', '--sandbox', '--no-token', 'notebook.py']shows the exact command arguments used to launch the notebook--headlessflag, you must manually navigate tohttp://0.0.0.0:27180.0.0.0just means it will work on all network hostsRun a notebook with external requirements dependencies file
1- Run the following Makefile command:
The running notebook should look and feel mostly identical, but you can see the presence of the requirements file
tests/fixtures/static_deps_reqs_txt/requirements.txtin the tests fixtures, and the arguments'--with-requirements', 'requirements.txt'in the logged command that was run.Discussion
We don't currently have a Github repository that we can use to test the cloning of a notebook repository. The above examples use the
--mountoption.But one can see that looking at the github repository cloning code that it's pretty straight-forward. We clone the repository and it becomes the root location, as if it existed already. We anticipate that most of our Marimo notebook repositories will have a
notebook.pyin the root of the repository and therefore won't even require a--pathflag orNOTEBOOK_PATHenv var. But, they can be as complex as needed, with utility files and dependencies as required.Includes new or updated dependencies?
YES
Changes expectations for external applications?
YES
What are the relevant tickets?