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

gh-100956: Update directions for how to run python + wasm #101003

Closed
wants to merge 1 commit into from

Conversation

brendandburns
Copy link

@brendandburns brendandburns commented Jan 13, 2023

Issue: #100956

Happy for feedback on better/different documentation.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jan 13, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@brettcannon brettcannon self-requested a review January 13, 2023 20:47
@brettcannon
Copy link
Member

@brendandburns FYI we will need you to sign the CLA before we can review this.

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A couple clarification suggestions and some ideas on how to simplify the script and make it more of a teachable moment. 🙂

Comment on lines +15 to +19
* [Build emscripten](https://github.com/python/cpython/tree/main/Tools/wasm#wasm32-emscripten-build)
* [Serve Python WASM](https://github.com/python/cpython/tree/main/Tools/wasm#hosting-python-wasm-builds)
* [Build WASI-SDK](https://github.com/python/cpython/tree/main/Tools/wasm#wasi-wasm32-wasi)
* [Running Python in WASI]((https://github.com/python/cpython/tree/main/Tools/wasm#running-python-wasi)

Copy link
Member

Choose a reason for hiding this comment

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

We should either use relative anchor links so this works offline or rely on GitHub's automatic table of contents for all Markdown files.

Comment on lines +291 to +292
Here is a simple script that can run a python script, it expects that the Python installation
is at the root of the filesystem (and it appears that currently that is [required](https://github.com/python/cpython/issues/100307).
Copy link
Member

Choose a reason for hiding this comment

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

Issue #100307 is not about where you run the WASI binary from, but how you have to mount the lib directory that contains the stdlib.

Suggested change
Here is a simple script that can run a python script, it expects that the Python installation
is at the root of the filesystem (and it appears that currently that is [required](https://github.com/python/cpython/issues/100307).
Here is a script for running a Python file. It helps demonstrate how WASI
follows a capabilities security model, requiring that you explicitly clear which
directories are accessible from within the WASI runtime. You can specify the
_host directories_ that you want to allow in and the _guest directories_ that
they map to.
Make sure to update the `PYTHON_WASI_ROOT` variable in the script to the
directory where you are storing your WASI binary and `lib` directories with the
stdlib.

FILE="/$PWD/${@#}"
fi

PYTHON_WASI_ROOT=/Python-3.11.0-wasm32-wasi-16
Copy link
Member

Choose a reason for hiding this comment

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

Should probably move this to the top and leave a comment about updating it.

Comment on lines +313 to +317
wasmtime run --dir ${PYTHON_WASI_ROOT} \
--mapdir /::${PYTHON_WASI_ROOT} \
--dir ${HOST_DIR} \
--mapdir ${GUEST_DIR}::${HOST_DIR} \
-- ${PYTHON_WASI_ROOT}/python.wasm $FILE
Copy link
Member

Choose a reason for hiding this comment

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

--mapdir automatically adds the host directories as accessible.

Suggested change
wasmtime run --dir ${PYTHON_WASI_ROOT} \
--mapdir /::${PYTHON_WASI_ROOT} \
--dir ${HOST_DIR} \
--mapdir ${GUEST_DIR}::${HOST_DIR} \
-- ${PYTHON_WASI_ROOT}/python.wasm $FILE
wasmtime run --mapdir /::${PYTHON_WASI_ROOT} \
--mapdir ${GUEST_DIR}::${HOST_DIR} \
-- ${PYTHON_WASI_ROOT}/python.wasm $FILE

REAL_PATH=$(realpath $1)
HOST_DIR=$(dirname $REAL_PATH)
GUEST_DIR=$HOST_DIR
FILE=${REAL_PATH}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe to help make things clearer around the difference between host and guest directories it's worth mapping to a /workspace guest directory?

# Launch a directory
python3-wasm some/directory/
```

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you just want to quickly launch the WASI binary, you can run
`wasmtime run --dir . python.wasm`. This does require putting any code you
want in the same directory as `python.wasm`. If you want to pass any
arguments, use `--`, e.g. `wasmtime run --dir . python.wasm -- -c "print('hello, world!')"`.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@brettcannon
Copy link
Member

Since this PR went stale, I implemented instructions in 70dc2fb .

@brettcannon brettcannon closed this Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants