-
Notifications
You must be signed in to change notification settings - Fork 4
fix(python-guest): persist module globals across run() calls #93
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
Open
simongdavies
wants to merge
1
commit into
hyperlight-dev:main
Choose a base branch
from
simongdavies:fix/python-guest-persist-globals
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+145
−2
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| //! Integration test: Python guest module globals persist across `run()`. | ||
| //! | ||
| //! This test pins the documented contract that the Python `Executor` | ||
| //! reuses one module-level namespace for every call to `run()` on the | ||
| //! same sandbox instance. The previous implementation built a fresh | ||
| //! `globals` dict on every call (`exec(code, {...})`), which silently | ||
| //! discarded any `def`, `class`, or top-level assignment between runs. | ||
| //! That contradicted: | ||
| //! | ||
| //! * the `WasmSandbox` `snapshot`/`restore` contract — the documented | ||
| //! mechanism for rewinding guest state — which only makes sense | ||
| //! if state otherwise survives a `run()` boundary; | ||
| //! * the `python_basics` example's "state was rolled back" narrative; | ||
| //! * the JavaScript guest, which preserves `globalThis` across runs. | ||
| //! | ||
| //! The tests below would have failed on the prior implementation; they | ||
| //! pass once `Executor` stores its globals on the instance and reuses | ||
| //! them across `run()` calls. | ||
|
|
||
| use std::path::Path; | ||
|
|
||
| use hyperlight_sandbox::SandboxBuilder; | ||
| use hyperlight_wasm_sandbox::Wasm; | ||
|
|
||
| fn python_guest_path() -> String { | ||
| Path::new(env!("CARGO_MANIFEST_DIR")) | ||
| .join("guests/python/python-sandbox.aot") | ||
| .display() | ||
| .to_string() | ||
| } | ||
|
|
||
| /// A `def` at module top level in `run()` #1 must be callable in `run()` #2. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the #2 here? |
||
| #[tokio::test] | ||
| async fn python_function_definition_persists_across_runs() { | ||
| let result = tokio::task::spawn_blocking(|| { | ||
| let mut sandbox = SandboxBuilder::new() | ||
| .guest(Wasm) | ||
| .module_path(python_guest_path()) | ||
| .build() | ||
| .expect("failed to create sandbox"); | ||
|
|
||
| sandbox | ||
| .run("def word_count(text): return len(text.split())") | ||
| .expect("first run failed"); | ||
|
|
||
| sandbox | ||
| .run("print(word_count('hello world from hyperlight'))") | ||
| .expect("second run failed") | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(result.exit_code, 0, "stderr: {}", result.stderr); | ||
| assert_eq!(result.stdout.trim(), "4"); | ||
| } | ||
|
|
||
| /// A bare module-level assignment in `run()` #1 must be readable in `run()` #2. | ||
| #[tokio::test] | ||
| async fn python_top_level_assignment_persists_across_runs() { | ||
| let result = tokio::task::spawn_blocking(|| { | ||
| let mut sandbox = SandboxBuilder::new() | ||
| .guest(Wasm) | ||
| .module_path(python_guest_path()) | ||
| .build() | ||
| .expect("failed to create sandbox"); | ||
|
|
||
| sandbox.run("counter = 100").expect("first run failed"); | ||
| sandbox | ||
| .run("print(f'counter = {counter}')") | ||
| .expect("second run failed") | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(result.exit_code, 0, "stderr: {}", result.stderr); | ||
| assert_eq!(result.stdout.trim(), "counter = 100"); | ||
| } | ||
|
|
||
| /// `snapshot` + `restore` must continue to rewind the persistent | ||
| /// namespace, undoing any names defined since the snapshot. This is | ||
| /// the contract documented on `WasmSandbox`; the persistence fix must | ||
| /// not regress it. | ||
| #[tokio::test] | ||
| async fn python_restore_rewinds_module_globals() { | ||
| let result = tokio::task::spawn_blocking(|| { | ||
| let mut sandbox = SandboxBuilder::new() | ||
| .guest(Wasm) | ||
| .module_path(python_guest_path()) | ||
| .build() | ||
| .expect("failed to create sandbox"); | ||
|
|
||
| let snap = sandbox.snapshot().expect("snapshot failed"); | ||
| sandbox | ||
| .run("rolled_back = 'still here'") | ||
| .expect("set failed"); | ||
| sandbox.restore(&snap).expect("restore failed"); | ||
|
|
||
| sandbox | ||
| .run( | ||
| r#" | ||
| try: | ||
| print(rolled_back) | ||
| except NameError: | ||
| print("rolled_back is undefined") | ||
| "#, | ||
| ) | ||
| .expect("post-restore run failed") | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(result.exit_code, 0, "stderr: {}", result.stderr); | ||
| assert_eq!(result.stdout.trim(), "rolled_back is undefined"); | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think we need to document what the "previous" implementation would have done