Make `pyodide venv` more compatible with standard `virtualenv` args + add tests #117

agriyakhetarpal · 2025-03-08T04:21:19Z

Description

This PR stems from #115 (comment) where I noticed that it is currently not possible to extend virtualenv args in order to override the virtual environment creation behaviour.

I've added a subset of the arguments listed at https://virtualenv.pypa.io/en/stable/cli_interface.html. The environment variables are already supported as we don't have much to deal with them, this PR enables passing those arguments. A warning is raised on currently and to-be-always unsupported arguments. For example, --no-pip does not make sense as we need pip to be able to install packages, and thus we always need a seeded environment.

It might be possible to add support for the --copies and --symlink-app-data flags as well, but I haven't looked into them as they seem to be a bit more complex than I expected. Also, I'm not sure if anyone will need the extra complexity.

Further steps that can be taken in the medium term are to create a virtualenv-pyodide plugin and register it with virtualenv so that we can remove venv.py out of pyodide-build altogether and maintain it separately. I would be happy to work on this, but will probably do it in a separate repository and shift it here.

Possible use cases:

allow installing a custom version of pip when creating the virtual environment, before it is patched up
disable pip version checks when creating an environment
embed extra packages into the environment at creation time if placed in a local directory
and so on

cc: @hoodmane

TODO:

Examine which args to add
Add CLI support
Documentation (most likely self-documented, so we don't need anything else)
Add some basic tests
- Figure out how to run the slow integration tests; maybe disable isolation to let them reuse the download xbuildenv

pyodide_build/out_of_tree/venv.py

agriyakhetarpal · 2025-03-08T04:27:03Z

pyodide_build/out_of_tree/venv.py

+    if virtualenv_args:
+        for arg in virtualenv_args:
+            if arg.startswith("--"):
+                # Check if the argument (or its prefix form) is supported.
+                arg_name = arg.split("=")[0] if "=" in arg else arg
+                if arg_name not in SUPPORTED_VIRTUALENV_OPTIONS:
+                    msg = f"Unsupported virtualenv option: {arg_name}"
+                    logger.warning(msg)
+
+        cli_args.extend(virtualenv_args)


Should we raise a warning or an error here?

pyodide_build/tests/test_venv.py

agriyakhetarpal · 2025-03-08T04:29:31Z

pyodide_build/tests/test_venv.py

+def test_pip_install(temp_venv_path, packages):
+    """Test that our monkeypatched pip can install packages into the venv"""


Instead of checking for our SYSCONFIGDATA patches and the like, I thought it would be better to see if the end-user functionality is working, as a working pip ensures that those patches have worked.

agriyakhetarpal · 2025-03-08T04:32:20Z

pyodide_build/out_of_tree/venv.py

+    # "--copies", "--always-copy", FIXME: node fails to invoke Pyodide
+    # "--symlink-app-data", FIXME: node fails to invoke Pyodide


Noting for completeness: for supporting these two arguments, we'll definitely need to patch the Pyodide Python (which I need to learn more about).

agriyakhetarpal · 2025-03-09T01:55:02Z

@ryanking13, I'm facing some trouble here where the dummy_xbuildenv and dummy_xbuildenv_url fixtures that we have in conftest.py are conflicting the test fixtures I introduced in the file, which causes the tests to fail when the test suite is run as a whole, but pass when you run the tests separately using -k test_venv or similar. I'm curious if you've come across such a situation before? I know about adding multiple conftest.py files, but I wouldn't like to do that as it will move a few files without any reason.

For context, I'm trying to test the venv creation with multiple options, as I noticed that it hasn't been tested before. It is easy to split pytest in CI into two steps, but I'm looking for something more robust so that testing locally remains intact.

Edit: either way, this PR is ready for review, it is only the tests that need to be fixed.

ryanking13 · 2025-03-09T02:03:24Z

I'm facing some trouble here where the dummy_xbuildenv and dummy_xbuildenv_url fixtures that we have in conftest.py are conflicting the test fixtures I introduced in the file

Hmm that's weird. Those two fixtures should create xbuildenv in a temporary directory which will be removed after the test function so I am not very sure how they can conflict each other.

Let me see if I can reproduce the error.

ryanking13 · 2025-03-09T02:24:52Z

I think I found the root cause. Let me open a PR to fix it.

@agriyakhetarpal

Tests that create xbuildenv didn't reset the cached functions, including the one that finds pyodide_root. It made the tests fail in #117 (comment) cc: @agriyakhetarpal

agriyakhetarpal · 2025-03-09T04:31:37Z

Thank you for the quick fix!

ryanking13 · 2025-03-09T06:28:31Z

Looks like the error got different. I guess we need to create a xbuildenv first to create a venv?

ryanking13 · 2025-03-09T06:36:04Z

Also, please note that the dummy_xbuildenv file that I made in this repository does not contain the python script or other wasm files to run the pyodide venv to reduce the size. I think you can update the dummy file to include those files, but it would increase the repository size, so there is a tradeoff.

Some other options can be:

Use mock files (with a very small or an empty file) for unittest, and instead use integration tests for testing the "real" venv functionality. In integration test, we may use the nightly xbuildenv file from the remote URL.
Testing the full functionality of pyodide venv in pyodide/pyodide, as the content inside dist directory varies per Pyodide version, not by pyodide-build.
Adopt pooch or other tools as you suggested in Remove binary files from repository micropip#165

agriyakhetarpal · 2025-03-09T13:20:07Z

Thanks, Gyeongjae. I'm currently downloading the latest (stable) Pyodide xbuildenv for the package installation tests, which get cached/reused between the tests so that we don't download it for every test. I don't intend to add the real xbuildenv tarball to the repository with this PR – that will be large, as you mention. The CI should pass now, but if you'd like me to label the slow tests as integration tests, I can do that.

agriyakhetarpal · 2025-03-09T13:28:27Z

Ah, I spoke too soon 😄 I'm not sure why it fails to find the cross-build environment for downloading. The tests are passing for me locally, though (I confirmed with a fresh state of the repository).

agriyakhetarpal · 2025-03-09T14:04:09Z

Okay, almost there – just one failing test (that one also passes on my machine) :D

agriyakhetarpal · 2025-03-09T22:37:54Z

The test fails because it uses the wrong Python interpreter to install packages. It, therefore, installs the macOS wheels for NumPy, and was silently passing in my case. To fix it, we need to call the Pyodide Python interpreter inside the venv (which relies on pyodide/pyodide#5448), or handle the activation/deactivation of the Pyodide venv inside that test to install the packages – I've proceeded with this approach for now in aca03ab.

I see that the rest of the pyodide venv functionality is also being tested elsewhere in Pyodide, in this file: https://github.com/pyodide/pyodide/blob/a53c17fd8ef87e79c7d3bfbb1262ca7ff6718ef2/src/tests/test_cmdline_runner.py, where the tests are more expansive. As @ryanking13 mentioned in pyodide/pyodide#5488, it makes sense to move the relevant tests from that file to pyodide-build, so that the functionality is better tested on every commit.

agriyakhetarpal · 2025-03-09T23:33:01Z

Argh, this passes locally and works perfectly fine, but not in CI...

agriyakhetarpal · 2025-03-10T00:49:31Z

So, it passes on macOS indeed, but it fails on Linux.

This reverts commit a75847f.

This reverts commit 822487c.

This reverts commit 08f3ef6.

This reverts commit 9d06537.

This reverts commit 9ceb978.

This reverts commit aca03ab.

agriyakhetarpal · 2025-03-10T02:25:42Z

🤔 no luck. I'm unable python -c "<...>" doesn't work on Linux. I'll probably drop this part of the test, as it is being tested elsewhere as mentioned in #117 (comment).

This reverts commit 8eb8d7d.

hoodmane · 2025-03-10T15:01:43Z

Do we have use cases for all these features? I'm a little reluctant to add the maintenance burden until someone actually wants the missing features. Currently, pyodide venv is just good enough for cibuildwheel. In particular, I'm concerned that we might implement the wrong thing, someone might start relying on it, and then we might have to change it later. Perhaps we could mark these options as experimental and subject to breaking changes depending on real world feedback?

agriyakhetarpal added 6 commits March 8, 2025 09:23

Add additional commands to pass to virtualenv

f093b6b

Add SUPPORTED_VIRTUALENV_OPTIONS

5d2226b

Support --clear and --no-clear

90e6341

Remove --no-wheel + split virtualenv's args

92ec586

Add some tests

efd2647

Note that a subset of args are supported

b0ead32

agriyakhetarpal commented Mar 8, 2025

View reviewed changes

agriyakhetarpal added 4 commits March 8, 2025 10:06

Add --symlinks arg (True by default)

2985410

Mention --pip can't be none (but don't validate)

76e6332

Add persistent xbuildenv for faster tests

e32a0cc

Trigger [integration] tests

b0664a8

agriyakhetarpal changed the title ~~[DRAFT]: Make pyodide venv more compatible with standard virtualenv args + add tests~~ Make pyodide venv more compatible with standard virtualenv args + add tests Mar 9, 2025

ryanking13 mentioned this pull request Mar 9, 2025

TST Reset cache after test #118

Merged

ryanking13 added a commit that referenced this pull request Mar 9, 2025

TST Reset cache after test (#118)

b9700c3

Tests that create xbuildenv didn't reset the cached functions, including the one that finds pyodide_root. It made the tests fail in #117 (comment) cc: @agriyakhetarpal

Merge branch 'main' into feat/virtualenv-compatibility-with-pyodide-venv

c5997ce

agriyakhetarpal marked this pull request as ready for review March 9, 2025 04:35

Fix tests, simplify xbuildenv preservation

c5d2246

agriyakhetarpal added 2 commits March 9, 2025 19:18

Switch to an editable installation

bd635ab

Infer the version properly

0613994

agriyakhetarpal added 2 commits March 10, 2025 03:35

Now it fails on my machine too

f68552a

Activate Pyodide virtual environment for tests.

aca03ab

agriyakhetarpal added 4 commits March 10, 2025 04:20

Insert debug points

9ceb978

Try activating Pyodide virtual env just once

9d06537

Add a few more debug points for GHA

08f3ef6

Add some more debugging?

822487c

Try out on macOS as well

a75847f

agriyakhetarpal added 8 commits March 10, 2025 06:32

Revert "Try out on macOS as well"

e5905f5

This reverts commit a75847f.

Revert "Add some more debugging?"

0c2ba14

This reverts commit 822487c.

Revert "Add a few more debug points for GHA"

25004fd

This reverts commit 08f3ef6.

Revert "Try activating Pyodide virtual env just once"

e353f5c

This reverts commit 9d06537.

Revert "Insert debug points"

3aa09f0

This reverts commit 9ceb978.

Revert "Activate Pyodide virtual environment for tests."

f1aa11f

This reverts commit aca03ab.

Install using pip, not using python -m pip

8cafb7e

Set PYTHONPATH

8eb8d7d

agriyakhetarpal added 2 commits March 10, 2025 07:55

Revert "Set PYTHONPATH"

52ee000

This reverts commit 8eb8d7d.

Don't try to import installed package (buggy test)

9ba7cfc

agriyakhetarpal requested review from ryanking13 and hoodmane March 10, 2025 02:31

Add a CHANGELOG entry for pyodide#117

3dc9854

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `pyodide venv` more compatible with standard `virtualenv` args + add tests #117

Make `pyodide venv` more compatible with standard `virtualenv` args + add tests #117

agriyakhetarpal commented Mar 8, 2025 •

edited

Loading

agriyakhetarpal Mar 8, 2025

agriyakhetarpal Mar 8, 2025

agriyakhetarpal Mar 8, 2025

agriyakhetarpal commented Mar 9, 2025 •

edited

Loading

ryanking13 commented Mar 9, 2025

ryanking13 commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

ryanking13 commented Mar 9, 2025

ryanking13 commented Mar 9, 2025 •

edited

Loading

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 10, 2025

agriyakhetarpal commented Mar 10, 2025

hoodmane commented Mar 10, 2025

		def test_pip_install(temp_venv_path, packages):
		"""Test that our monkeypatched pip can install packages into the venv"""

		# "--copies", "--always-copy", FIXME: node fails to invoke Pyodide
		# "--symlink-app-data", FIXME: node fails to invoke Pyodide

Make pyodide venv more compatible with standard virtualenv args + add tests #117

Are you sure you want to change the base?

Make pyodide venv more compatible with standard virtualenv args + add tests #117

Conversation

agriyakhetarpal commented Mar 8, 2025 • edited Loading

Description

agriyakhetarpal Mar 8, 2025

Choose a reason for hiding this comment

agriyakhetarpal Mar 8, 2025

Choose a reason for hiding this comment

agriyakhetarpal Mar 8, 2025

Choose a reason for hiding this comment

agriyakhetarpal commented Mar 9, 2025 • edited Loading

ryanking13 commented Mar 9, 2025

ryanking13 commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

ryanking13 commented Mar 9, 2025

ryanking13 commented Mar 9, 2025 • edited Loading

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 9, 2025

agriyakhetarpal commented Mar 10, 2025

agriyakhetarpal commented Mar 10, 2025

hoodmane commented Mar 10, 2025

Make `pyodide venv` more compatible with standard `virtualenv` args + add tests #117

Make `pyodide venv` more compatible with standard `virtualenv` args + add tests #117

agriyakhetarpal commented Mar 8, 2025 •

edited

Loading

agriyakhetarpal commented Mar 9, 2025 •

edited

Loading

ryanking13 commented Mar 9, 2025 •

edited

Loading