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

Support serving static files through symlinks #1298

Closed
Ubehebe opened this issue May 2, 2024 · 9 comments
Closed

Support serving static files through symlinks #1298

Ubehebe opened this issue May 2, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@Ubehebe
Copy link
Contributor

Ubehebe commented May 2, 2024

Describe the bug

Hi maintainers, thanks for your work on this project. I'm really interested to get it working on my machine; it looks like it could improve my workflow significantly.

I'm trying to get marimo working with my Bazel monorepo via https://github.com/bazelbuild/rules_python. Among other things, Bazel makes extensive use of symlinks to provide an executable's runtime dependencies, including data files inside a pip package.

The failure mode I'm currently observing is that the marimo server boots fine and serves index.html, but all of the static assets referenced from index.html are 404s.

After a bit of debugging, I believe this is due to how marimo sets up its static files: https://github.com/marimo-team/marimo/blob/main/marimo/_server/api/endpoints/assets.py#L34-L38:

router.mount(
    "/assets",
    app=StaticFiles(directory=os.path.join(root, "assets")),
    name="assets",
)

The documentation for StaticFiles states:

follow_symlink - A boolean indicating if symbolic links for files and directories should be followed. Defaults to False.

I believe adding follow_symlink=True to the router.mount() invocation above will allow me to use marimo with Bazel's symlinked filesystem.

I'll try to confirm tomorrow by patching.

Thanks again for your time!

Environment

{
  "marimo": "0.4.10",
  "OS": "Darwin",
  "OS Version": "23.4.0",
  "Processor": "arm",
  "Python Version": "3.11.4",
  "Binaries": {
    "Browser": "124.0.6367.93",
    "Node": "v21.7.3"
  },
  "Requirements": {
    "click": "8.1.3",
    "importlib-resources": "missing",
    "jedi": "0.18.2",
    "markdown": "3.6",
    "pymdown-extensions": "10.8.1",
    "pygments": "2.15.1",
    "tomlkit": "0.12.4",
    "uvicorn": "0.29.0",
    "starlette": "0.37.2",
    "websocket": "missing",
    "typing-extensions": "missing",
    "black": "23.3.0"
  }
}

Code to reproduce

No response

@Ubehebe Ubehebe added the bug Something isn't working label May 2, 2024
@mscolnick
Copy link
Contributor

when a reproducible build system meets a reproducible notebook 🤝

@mscolnick
Copy link
Contributor

But this change seems fine, we can change this tomorrow unless you'd like the contribution?

@Ubehebe
Copy link
Contributor Author

Ubehebe commented May 2, 2024

I'll make this change in a local fork tomorrow morning and confirm that it fixes my problem. If you fix it before then, that's fine too. Thanks!

@mscolnick
Copy link
Contributor

There is some similar issues with bazel and starlette-based frameworks. Starlette did fix a security issue, but to be safe, it seems worth making this configurable.

We can add this to our MarimoConfig (in python) which is a marimo.toml file on your computer. If you are using a pyproject.toml, we've been meaning to move project-level config there, so we can do that work for this.

encode/starlette#1083
Chainlit/chainlit#317

@dmadisetti
Copy link
Collaborator

So I actually use bazel

in my build

py_binary(                                                                                                                                                                                                                                    
    name = "marimo-server",                                                                                                                                                                                                                   
    srcs = ["marimo-server.py"],                                                                                                                                                                                                              
    srcs_version = "PY3",                                                                                                                                                                                                                     
    deps = [                                                                                                                                                                                                                                  
        "//project",
        requirement("jupyterlab"),                                                                                                                                                                                                            
        requirement("notebook"),
        # whatever                                                                                                                                                                                                            
    ],                                                                                                                                                                                                                                        
)                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      
                                                                                                                                                                                                                                              
sh_binary(                                                                                                                                                                                                                                    
    name = "marimo",                                                                                                                                                                                                                          
    srcs = ["marimo.sh"],                                                                                                                                                                                                                     
    data = [":marimo-server"],                                                                                                                                                                                                                
)

in server

#!/usr/bin/env bash
export RUNFILES_DIR=$(dirname `pwd`)
BUILD_PATH=$RUNFILES_DIR/project
BUILD_PATH=`pwd`

cd $(dirname `realpath notebooks/marimo.sh`)

RUNFILES_DIR=$RUNFILES_DIR \
SHELL=$(which bash) \
$BUILD_PATH/notebooks/marimo-server $@

and marimo-server:

import re
import sys
from marimo._cli.cli import main

if __name__ == "__main__":
    sys.argv[0] = re.sub(r"(-script\.pyw|\.exe)?$", "", sys.argv[0])
    sys.exit(main())

I vaguely remember having the symlink issue and asking on discord, but this works

@dmadisetti
Copy link
Collaborator

dmadisetti commented May 2, 2024

Hmm, I don't remember why this works. I think it's because I bundle marimo into python, or maybe the $RUNFILES

I've come across similar issues before and some projects consider follow symlink to be a security risk, so just food for thought.


Found it!

https://discord.com/channels/1059888774789730424/1059888774789730427/1225179457707446386

Yeah, I think this is not as easy as it seems first glance

@Ubehebe
Copy link
Contributor Author

Ubehebe commented May 2, 2024

Confirmed that the following patch fixes the marimo server in bazel:

--- a/marimo/_server/api/endpoints/assets.py
+++ b/marimo/_server/api/endpoints/assets.py
@@ -33,7 +33,7 @@ root = os.path.realpath(str(import_files("marimo").joinpath("_static")))
 
 router.mount(
     "/assets",
-    app=StaticFiles(directory=os.path.join(root, "assets")),
+    app=StaticFiles(directory=os.path.join(root, "assets"), follow_symlink=True),
     name="assets",
 )

(For the curious, rules_python has a nifty wheel-patching API that allowed me to experiment without rebuilding marimo.)

@mscolnick I'll try to follow your suggestion for adding this to MarimoConfig. Hopefully can send a PR tomorrow.

@mscolnick
Copy link
Contributor

@Ubehebe thanks for confirming

Ubehebe added a commit to Ubehebe/marimo that referenced this issue May 7, 2024
mscolnick pushed a commit that referenced this issue May 7, 2024
* draft: potential fix for #1298.

* update snapshot tests
@mscolnick
Copy link
Contributor

This was fixed on #1327 (thanks @Ubehebe!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants