Skip to content
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

allow --global-session for marimo run #2489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

peterwilli
Copy link
Contributor

📝 Summary

I encountered this use-case here: #2488

When doing large computations running entirely locally, especially with non-coder users, it is often important to be able to resume the session when the browser refreshes, or tabs wake up from sleep, especially when loading large files into memory.

In my case, a customer has refreshed their browser, causing a large model to load in memory twice. We don't run marimo on a server but locally, so there's no need for unique sessions.

This feature lets you add --global-session to your marimo run command, sharing the session even when refreshing, asserting the same session behaviour as marimo edit.

For the rest, default behaviour stays, so after this PR, everyone still gets unique sessions in marimo run, except if they provide the argument.

This is a living PR, and certainly not perfect yet - I assume that we have to add some e2e tests, but I'm not familiar enough to make this work yet, and need some guidance.

🔍 Description of Changes

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

Copy link

vercel bot commented Oct 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 2:03pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 2:03pm

@mscolnick
Copy link
Contributor

this is great! the direction looks right - could we verify with some e2e tests? some examples:

  1. only one connection is allowed when in --global-session. otherwise we will likely get some bugs that there is inconsistent state for different users. (state would exist on the client and server that may not match)
  2. we can resume the session while in run mode
  3. we probably already have a test for this, but just checking that 2 run modes can be active at once. test_ws.py is the place to find these

could you also help update docs/guides/app.md? we could add a section ## Global session

@@ -661,6 +661,7 @@ def __init__(
cli_args: SerializedCLIArgs,
auth_token: Optional[AuthToken],
redirect_console_to_browser: bool = False,
global_session: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can rename to global_session_in_run_mode or something similar. it is verbose, but this could be confused that edit mode also as global session configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah certainly, any name that makes sense is good for me :)

@peterwilli
Copy link
Contributor Author

peterwilli commented Oct 3, 2024

I looked at the e2e tests, and it seems the edit test does this by checking presence of the message banner:

test("can resume a session", async ({ page }) => {
  const appUrl = getAppUrl("shutdown.py");
  await page.goto(appUrl);

  await expect(page.getByText("None", { exact: true })).toBeVisible();
  // type in the form
  await page.locator("#output-Hbol").getByRole("textbox").fill("12345");
  // shift enter to run the form
  await page.keyboard.press("Meta+Enter");

  // wait for the output to appear
  let secondCell = await page.locator(".Cell").nth(1);
  await expect(secondCell.getByText("12345")).toBeVisible();
  await expect(secondCell.getByText("54321")).toBeVisible();

  // Refresh the page
  await page.reload();

  await expect(
    page.getByText("You have reconnected to an existing session."),
  ).toBeVisible();
  secondCell = await page.locator(".Cell").nth(1);
  await expect(page.getByText("12345")).toBeVisible();
  await expect(page.getByText("54321")).toBeVisible();
});

My PR doesn't display such banner, so I guess I have to add that same banner to make sure we can run a similar test

@mscolnick
Copy link
Contributor

sorry i mispoke, you can just add the tests to the python side. our e2e tests with playright are a bit finicky and flakey so we don't keep those updated.

@peterwilli
Copy link
Contributor Author

sorry i mispoke, you can just add the tests to the python side. our e2e tests with playright are a bit finicky and flakey so we don't keep those updated.

Ok, I'm currently checking out the Python tests, how they work etc

@peterwilli
Copy link
Contributor Author

@mscolnick I looked into the test_ws.py, I see how my test would fit there, but I'm struggling to understand where the server is launched, is there some place where the tests are initated?

@mscolnick
Copy link
Contributor

@peterwilli - an marimo server is not launched in those tests, but rather we use the starlette TestClient https://www.starlette.io/testclient/

@mscolnick
Copy link
Contributor

@peterwilli, can we help unblock you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants