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

Proof of concept of bindgen tests using componentize-py #234

Merged
merged 2 commits into from
May 15, 2024

Conversation

jamesls
Copy link
Contributor

@jamesls jamesls commented May 8, 2024

This is a sketch of the idea suggested in #232 (review) where instead of hand writing wat files, you can instead write a wit file and corresponding guest code in Python, and we then use componentize-py to generate a wasm component for which we then generate and test host bindings via wasmtime.bindgen.

Not intending for this to be merged, just putting something up to start a discussion.

I ported a couple of tests from tests/codegen to get a sense of what these tests look like (these are in tests/bindgen/test_bindgen.py).

Essentially, create a subdirectory under tests/bindgen with the wit/guest code:

bare_funcs/
├── app.py          <-- guest code implementation
├── barefuncs       <-- componentize-py bindings
│   ├── __init__.py
│   └── types.py
└── component.wit   <-- test .wit file

Wit file:

package component:barefuncs;

world barefuncs {
  export foo: func(a: s32) -> s32;
}

Guest code:

class Barefuncs:
    def foo(self, a: int) -> int:
        return a + 1

You can then write a test for this using:

from . import BindgenTestCase, generate_bindings

def test_bare_funcs():
    testcase = BindgenTestCase(
        guest_code_dir='bare_funcs',
        world_name='barefuncs',
    )
    store, root = generate_bindings(testcase)
    assert root.foo(store, 10) == 11

The generate_bindings() will build the wasm component via componentize-py, generate bindings using wasmtime.bindgen, and then instantiate and return the root module back to the caller for testing (probably would want this to be optional).

These tests are substantially slower than the existing tests (which we'd expect). These take about 20-30 seconds on my machine with just the three tests ported over. Even if we implement some mtime cache to skip the componentize-py generation, the tests still take around 5-10 seconds. I don't think this is a deal breaker necessarily, but something to consider.

Let me know what you think. I can flesh the rest of this out if this seems like something worth pursuing.

@jamesls jamesls force-pushed the new-bindgen-tests branch from 0be271c to 95a22ab Compare May 8, 2024 21:58
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This looks awesome, thank you so much for pushing on this!

the tests still take around 5-10 seconds

One thing that might help a lot here is Wasmtime's incremental compilation feature. That being said this isn't currently exposed in the C API, but in Rust it would look something like this where all tests are derived from the same Engine which has one large incremental cache.

If there's a significant amount of time taken in componentize-py though that wouldn't help much since that's just the wasmtime compilation side of things. @dicej do you have any suggestions for optimizing the runtime here for componentize-py perhaps?

raise RuntimeError("Could not find componentize-py executable.")
with tempfile.NamedTemporaryFile('w') as f:
output_wasm = str(f.name + '.wasm')
with chdir(testcase.app_dir):
Copy link
Member

Choose a reason for hiding this comment

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

Since the cwd is a global property of the process could this be done just for the subprocess.run call with the cwd keyword argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get that updated, this was leftover from when I was importing componentize_py directly, but I was running into issues with it affecting subsequent tests, so switching to an external process fixed the issue.

Comment on lines 3 to 9
def test_bare_funcs():
testcase = BindgenTestCase(
guest_code_dir='bare_funcs',
world_name='barefuncs',
)
store, root = generate_bindings(testcase)
assert root.foo(store, 10) == 11
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to put these individual tests within each folder? For example keeping everything self-contained within each sub-folder to avoid a large listing of all tests here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I can get that working. I had to configure pytest to ignore these directories to avoid issues with module name conflicts with app.py, but there might be a way around this. Worst case, we can split them out into separate files in this directory, similar to the existing structure in codegen/.

@dicej
Copy link
Contributor

dicej commented May 9, 2024

Creating a component with componentize-py currently takes a bit of time, almost all of which is spent compiling the 30+ MB of Wasm code to native code so it can be pre-initialized. The pre-initialization step is kind of mandatory because that's the last chance we have to load Python code from the filesystem (e.g. standard library and dependency code). This is one of the reasons I've been eager to try Winch, but I've been waiting for ARM support so far. But maybe incremental compilation could help with that, too?

Another approach might be to add an option to componentize-py that tells it to skip pre-initialization and instead ensure that the standard library and dependency code are available via WASI preopens at runtime.

@alexcrichton
Copy link
Member

Ah makes sense! Incremental compilation would help in this case I think because the python.wasm across multiple invocations of componentize-py are probably all the same or very similar. That would require cross-process caching though which could get nontrivial.

Could componentize-py grow a flag for which which could be opted-in-to on x64 perhaps? That might be good to test here and see if it helps

@dicej
Copy link
Contributor

dicej commented May 9, 2024

Could componentize-py grow a flag for which which could be opted-in-to on x64 perhaps?

Yeah, or even just add something like

if cfg!(target_arch = "x86_64") {
    config.strategy(Strategy::Winch);
}

to https://github.com/bytecodealliance/componentize-py/blob/c6c8447db66f5de66671a6b57ad47c61cb094af8/src/lib.rs#L718

@dicej
Copy link
Contributor

dicej commented May 9, 2024

BTW, the trick I use in componentize-py's own test suite is to create one single component which can run all of the tests (i.e. it targets this big fat world: https://github.com/bytecodealliance/componentize-py/blob/main/src/test/wit/tests.wit)

jamesls added 2 commits May 14, 2024 13:26
The basic idea is that you define a wit file along with guest code
in Python.  We can then generate a wasm component using componentize-py,
host bindings using `wasmtime.bindgen`, and then test that the generated
bindings work as expected.
* Each test case is now in the same directory with the corresponding
  WIT file and guest code (app.py).
* The helper functions for generating wasm components and host bindings
  is moved to a pytest fixture to avoid relative package issues when
  using direct imports.
* Remove the use of changing the cwd in the test process.
@jamesls jamesls force-pushed the new-bindgen-tests branch from a429569 to 08ca78b Compare May 14, 2024 17:28
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks again very much for this! I like the way this is shaping up and I'm happy enough with this that we can handle the slow-tests part down-the-road.

@alexcrichton alexcrichton merged commit 3ac42c4 into bytecodealliance:main May 15, 2024
11 checks passed
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.

3 participants