feat(engine): add WasmRuntimeExecutor for attributes
#636
Conversation
WalkthroughThe pull request introduces significant modifications to the CI workflow and various components of the codebase to integrate Python support. Key changes include the addition of Python 3.11 and the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
soma00333
changed the title
WasmRuntimeExecutor for attributes
| "ProcessorType": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "Attribute" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Support only Attribute in this scope.
| "ProgrammingLanguage": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "Python" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Support only Python in this scope.
| fn compile_to_wasm(&self, params: &WasmRuntimeExecutorParam) -> super::errors::Result<()> { | ||
| match params.programming_language { | ||
| ProgrammingLanguage::Python => { | ||
| let py2wasm_output = std::process::Command::new("py2wasm") | ||
| .args([¶ms.source_code_file_path, "-o", ¶ms.wasm_file_path]) | ||
| .output() | ||
| .map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutorFactory(format!( | ||
| "Failed to run py2wasm: {}. Command: py2wasm {} -o {}", | ||
| e, params.source_code_file_path, params.wasm_file_path | ||
| )) | ||
| })?; | ||
|
|
||
| if !py2wasm_output.status.success() { | ||
| let error_msg = String::from_utf8_lossy(&py2wasm_output.stderr); | ||
| return Err(WasmProcessorError::RuntimeExecutorFactory(format!( | ||
| "Python compilation failed: {}", | ||
| error_msg | ||
| ))); | ||
| } | ||
| } | ||
| _ => { | ||
| return Err(WasmProcessorError::RuntimeExecutorFactory( | ||
| "Unsupported programming language".to_string(), | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Compile python scripts to .wasm with py2wasm on python 3.11
| fn process_attribute( | ||
| &self, | ||
| ctx: ExecutorContext, | ||
| fw: &mut dyn ProcessorChannelForwarder, | ||
| ) -> super::errors::Result<()> { | ||
| let mut feature = ctx.feature.clone(); | ||
|
|
||
| let json_input = self.serialize_attributes(&feature.attributes)?; | ||
| let output = self.execute_wasm_module(&json_input)?; | ||
| let updated_attributes = self.parse_wasm_output(&output)?; | ||
|
|
||
| feature.attributes = updated_attributes; | ||
| fw.send(ctx.new_with_feature_and_port(feature.clone(), DEFAULT_PORT.clone())); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Process logic for processor_type: attribute
| fn serialize_attributes( | ||
| &self, | ||
| attributes: &HashMap<Attribute, AttributeValue>, | ||
| ) -> super::errors::Result<String> { | ||
| serde_json::to_string(attributes).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!( | ||
| "Failed to serialize Feature to JSON: {}", | ||
| e | ||
| )) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Serialize attributes to json
| fn execute_wasm_module(&self, input: &str) -> super::errors::Result<String> { | ||
| let wasm_bytes = std::fs::read(&self.params.wasm_file_path).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!( | ||
| "Failed to read Wasm file '{}': {}", | ||
| &self.params.wasm_file_path, e | ||
| )) | ||
| })?; | ||
|
|
||
| let mut store = Store::default(); | ||
| let module = Module::new(&store, wasm_bytes).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!("Failed to compile module: {}", e)) | ||
| })?; | ||
|
|
||
| let (stdout_tx, mut stdout_rx) = Pipe::channel(); | ||
| WasiEnv::builder("") | ||
| .args(&[input]) | ||
| .stdout(Box::new(stdout_tx)) | ||
| .run_with_store(module, &mut store) | ||
| .map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!("Failed to execute module: {}", e)) | ||
| })?; | ||
|
|
||
| let mut output = String::new(); | ||
| stdout_rx.read_to_string(&mut output).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!( | ||
| "Failed to read stdout from Wasm module: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| Ok(output) | ||
| } |
There was a problem hiding this comment.
Execute .wasm with wasmer and wasmer_wasix
| fn parse_wasm_output( | ||
| &self, | ||
| output: &str, | ||
| ) -> super::errors::Result<HashMap<Attribute, AttributeValue>> { | ||
| const STATUS_SUCCESS: &str = "success"; | ||
| const STATUS_ERROR: &str = "error"; | ||
| const FIELD_ATTRIBUTES: &str = "attributes"; | ||
| const FIELD_ERROR: &str = "error"; | ||
| const FIELD_STATUS: &str = "status"; | ||
|
|
||
| let parsed_output: serde_json::Value = serde_json::from_str(output).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!( | ||
| "Failed to parse Wasm module output as JSON: {}", | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| match parsed_output.get(FIELD_STATUS) { | ||
| Some(serde_json::Value::String(status)) if status == STATUS_SUCCESS => { | ||
| if let Some(attributes_value) = parsed_output.get(FIELD_ATTRIBUTES) { | ||
| serde_json::from_value(attributes_value.clone()).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!( | ||
| "Failed to deserialize '{}': {}", | ||
| FIELD_ATTRIBUTES, e | ||
| )) | ||
| }) | ||
| } else { | ||
| Err(WasmProcessorError::RuntimeExecutor(format!( | ||
| "Missing '{}' in Wasm module output", | ||
| FIELD_ATTRIBUTES | ||
| )) | ||
| .into()) | ||
| } | ||
| } | ||
| Some(serde_json::Value::String(status)) if status == STATUS_ERROR => { | ||
| let error_msg = match parsed_output.get(FIELD_ERROR) { | ||
| Some(serde_json::Value::String(err)) => { | ||
| format!("Wasm module execution failed with error: {}", err) | ||
| } | ||
| _ => "Wasm module execution failed with an unknown error.".to_string(), | ||
| }; | ||
| Err(WasmProcessorError::RuntimeExecutor(error_msg).into()) | ||
| } | ||
| _ => Err(WasmProcessorError::RuntimeExecutor(format!( | ||
| "Unexpected '{}' in Wasm module output", | ||
| FIELD_STATUS | ||
| )) | ||
| .into()), | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle errors of output and deserialize to attributes
| # yaml-language-server: $schema=https://raw.githubusercontent.com/reearth/reearth-flow/main/engine/schema/workflow.json | ||
| id: 00caad2a-9f7d-4189-b479-153fa9ea36dc | ||
| name: "WasmRuntimeExecutorExample" | ||
| entryGraphId: 3e3450c8-2344-4728-afa9-5fdb81eec33a | ||
| graphs: | ||
| - id: 3e3450c8-2344-4728-afa9-5fdb81eec33a | ||
| name: entry_point | ||
| nodes: | ||
| - id: 90f40a3e-61d3-48e2-a328-e7226c2ad1ae | ||
| name: FeatureCreator | ||
| type: action | ||
| action: FeatureCreator | ||
| with: | ||
| creator: | | ||
| [ | ||
| #{ | ||
| attribute1: "test01", | ||
| attribute2: "test02" | ||
| } | ||
| ] | ||
|
|
||
| - id: F3F168F4-1D7B-41E5-B6A2-0FCA96ED10D6 | ||
| name: WasmRuntimeExecutor | ||
| type: action | ||
| action: WasmRuntimeExecutor | ||
| with: | ||
| processorType: "Attribute" | ||
| programmingLanguage: "Python" | ||
| sourceCodeFilePath: "runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py" | ||
| wasmFilePath: "runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.wasm" | ||
|
|
||
| - id: 756F498A-7A5C-4579-AB1C-FCBBA8121869 | ||
| name: EchoSink | ||
| type: action | ||
| action: EchoSink | ||
|
|
||
| edges: | ||
| - id: c064cf52-705f-443a-b2de-6795266c540d | ||
| from: 90f40a3e-61d3-48e2-a328-e7226c2ad1ae | ||
| to: F3F168F4-1D7B-41E5-B6A2-0FCA96ED10D6 | ||
| fromPort: default | ||
| toPort: default | ||
|
|
||
| - id: 7A963683-4E97-4B75-9BDC-BE6B3953003B | ||
| from: F3F168F4-1D7B-41E5-B6A2-0FCA96ED10D6 | ||
| to: 756F498A-7A5C-4579-AB1C-FCBBA8121869 | ||
| fromPort: default | ||
| toPort: default |
| def process_attributes(attributes): | ||
| updated_attributes = {} | ||
| for key, value in attributes.items(): | ||
| if isinstance(value, dict) and "String" in value: | ||
| updated_attributes[key] = {"String": value["String"] + "_modified"} | ||
| else: | ||
| updated_attributes[key] = value | ||
| return updated_attributes |
There was a problem hiding this comment.
"XXX" to "XXX_modified"
| def main(): | ||
| try: | ||
| input_data = sys.argv[1] | ||
| parsed_data = json.loads(input_data) | ||
|
|
||
| updated_attributes = process_attributes(parsed_data) | ||
|
|
||
| output = {"status": "success", "attributes": updated_attributes, "error": None} | ||
| except Exception as e: | ||
| output = {"status": "error", "attributes": None, "error": str(e)} | ||
|
|
||
| print(json.dumps(output), file=sys.stdout) |
There was a problem hiding this comment.
Expects output of {"status":XXX, "attributes":YYY, "error":ZZZ}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (12)
engine/runtime/action-processor/src/wasm/runtime_executor.rs (1)
187-192: Correct the error message inserialize_attributesmethodIn the
serialize_attributesmethod, the error message incorrectly states "Failed to serialize Feature to JSON," whereas it is actually serializing attributes. Consider updating the error message to accurately reflect the data being serialized:- WasmProcessorError::RuntimeExecutor(format!( - "Failed to serialize Feature to JSON: {}", + WasmProcessorError::RuntimeExecutor(format!( + "Failed to serialize attributes to JSON: {}", e ))engine/runtime/tests/testcase/wasm/runtime_executor.rs (1)
1-6: Consider using test constants for pathsThe hardcoded paths could be moved to constants at the module level for better maintainability.
const TEST_DATA_DIR: &str = "wasm/runtime_executor/attribute_python"; const EXPECT_FILE: &str = "expect.json"; #[test] fn test_attribute_python() { execute_with_test_assert(TEST_DATA_DIR, EXPECT_FILE); }engine/runtime/action-processor/src/wasm/errors.rs (1)
5-10: Consider enhancing error handling with more specific error types.The current implementation uses String for error details, which could be improved:
- Consider using more specific error types instead of String to provide better error handling:
- The error messages could be more detailed to help with debugging.
Here's a suggested improvement:
#[derive(Error, Debug)] pub(super) enum WasmProcessorError { - #[error("Wasm Runtime Executor Factory error: {0}")] - RuntimeExecutorFactory(String), - #[error("Wasm Runtime Executor error: {0}")] - RuntimeExecutor(String), + #[error("Failed to create Wasm Runtime Executor: {kind}")] + RuntimeExecutorFactory { + kind: RuntimeExecutorFactoryErrorKind, + #[source] + source: Option<Box<dyn std::error::Error + Send + Sync>>, + }, + #[error("Error during Wasm execution: {kind}")] + RuntimeExecutor { + kind: RuntimeExecutorErrorKind, + #[source] + source: Option<Box<dyn std::error::Error + Send + Sync>>, + }, } +#[derive(Debug)] +pub(super) enum RuntimeExecutorFactoryErrorKind { + #[error("Invalid configuration: {0}")] + InvalidConfig(String), + #[error("Failed to initialize runtime: {0}")] + InitializationError(String), +} + +#[derive(Debug)] +pub(super) enum RuntimeExecutorErrorKind { + #[error("Memory allocation failed: {0}")] + MemoryError(String), + #[error("Execution failed: {0}")] + ExecutionError(String), +}engine/runtime/action-processor/Cargo.toml (1)
50-51: Consider documenting WebAssembly security guidelines.Since this introduces WebAssembly execution capabilities, consider adding documentation about:
- Secure configuration of the Wasmer runtime
- Allowed WASI capabilities and their security implications
- Best practices for sandboxing WebAssembly modules
Would you like me to help draft security guidelines for the WebAssembly integration?
engine/runtime/examples/plateau/testdata/workflow/wasm_runtime_executor.yml (2)
1-1: Consider pinning the schema reference to a specific versionUsing the main branch for schema validation could lead to unexpected behavior if the schema changes. Consider pinning to a specific version or commit hash for better stability.
-# yaml-language-server: $schema=https://raw.githubusercontent.com/reearth/reearth-flow/main/engine/schema/workflow.json +# yaml-language-server: $schema=https://raw.githubusercontent.com/reearth/reearth-flow/v1.0.0/engine/schema/workflow.json
1-48: Consider enhancing workflow resilienceAs this workflow serves as an example for WASM execution, consider:
- Adding error handling paths for WASM execution failures
- Including timeout configurations for the execution
- Adding validation steps before and after WASM execution
- Documenting memory/resource requirements for WASM execution
engine/containers/cli/Dockerfile (1)
52-59: Consider security implications of Python installation.Adding Python and pip to the final image increases the attack surface. Consider:
- Creating a separate Python build stage for py2wasm compilation
- Copying only the necessary wasm files to the final stage
Would you like assistance in restructuring the Dockerfile to use multi-stage builds for Python dependencies?
engine/containers/worker/Dockerfile (1)
52-58: Consider multi-stage build for Python compilation.To optimize the final image size, consider:
- Moving the Python/py2wasm environment to a separate build stage
- Only copying the compiled .wasm files to the final stage
- Using a dedicated Python builder stage for WebAssembly compilation
This would significantly reduce the attack surface and final image size.
Example structure:
# Python builder stage FROM python:3.11-slim AS wasm-builder RUN pip install "py2wasm==0.1.0" COPY scripts /scripts RUN py2wasm /scripts/*.py -o /wasm/ # Final stage FROM debian:bookworm-slim COPY --from=wasm-builder /wasm/*.wasm /wasm/engine/runtime/tests/fixture/workflow/wasm/runtime_executor/attribute_python.yaml (2)
16-22: Consider parameterizing test attributesThe hardcoded test values limit test coverage. Consider parameterizing these values through workflow parameters to enable testing different scenarios.
creator: | [ #{ - attribute1: "test01", - attribute2: "test02" + attribute1: env.get("test_attribute1", "test01"), + attribute2: env.get("test_attribute2", "test02") } ]
1-54: Consider additional test workflows for edge casesWhile this workflow effectively demonstrates the basic functionality, consider adding additional test workflows to cover:
- Error handling scenarios (invalid WASM, compilation failures)
- Large data processing (performance testing)
- Memory constraints and resource usage
- Concurrent execution scenarios
This will help ensure the robustness of the WasmRuntimeExecutor implementation.
.github/workflows/ci_engine.yml (1)
53-56: Consider pinning py2wasm version and verifying installation.While the installation steps are correct, consider these improvements:
- Pin the py2wasm version for reproducible builds
- Add a verification step to ensure successful installation
Here's a suggested improvement:
- name: Install py2wasm run: | python -m pip install --upgrade pip - pip install py2wasm + pip install py2wasm==1.0.0 # Replace with your desired version + # Verify installation + python -c "import py2wasm; print(f'py2wasm version: {py2wasm.__version__}')"engine/schema/actions.json (1)
2673-2726: The WasmRuntimeExecutor processor definition looks good but could be enhanced.The basic structure and parameter schema are well-defined, matching the PR objectives. However, consider the following improvements:
- Add an error output port to handle compilation and execution failures gracefully
- Add validation patterns for file paths
- Consider adding optional runtime configuration parameters (e.g., memory limits, timeout)
Apply this diff to enhance the processor definition:
{ "name": "WasmRuntimeExecutor", "type": "processor", "description": "Compiles scripts into .wasm and runs at the wasm runtime", "parameter": { "$schema": "http://json-schema.org/draft-07/schema#", "title": "WasmRuntimeExecutorParam", "type": "object", "required": [ "processorType", "programmingLanguage", "sourceCodeFilePath", "wasmFilePath" ], "properties": { "processorType": { "$ref": "#/definitions/ProcessorType" }, "programmingLanguage": { "$ref": "#/definitions/ProgrammingLanguage" }, "sourceCodeFilePath": { - "type": "string" + "type": "string", + "pattern": "^[^<>:;,?\"*|/]+$" }, "wasmFilePath": { - "type": "string" + "type": "string", + "pattern": "^[^<>:;,?\"*|/]+$" }, + "memoryLimit": { + "type": ["integer", "null"], + "minimum": 1, + "maximum": 4096, + "description": "Memory limit in MB" + }, + "timeout": { + "type": ["integer", "null"], + "minimum": 1, + "maximum": 300, + "description": "Execution timeout in seconds" + } }, "definitions": { "ProcessorType": { "type": "string", "enum": [ "Attribute" ] }, "ProgrammingLanguage": { "type": "string", "enum": [ "Python" ] } } }, "builtin": true, "inputPorts": [ "default" ], "outputPorts": [ "default", + "error" ], "categories": [ "Wasm" ] },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
engine/Cargo.lockis excluded by!**/*.lock,!**/*.lockengine/README.mdis excluded by!**/*.mdengine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (19)
.github/workflows/ci_engine.yml(1 hunks)engine/Cargo.toml(1 hunks)engine/containers/cli/Dockerfile(1 hunks)engine/containers/worker/Dockerfile(1 hunks)engine/runtime/action-processor/Cargo.toml(1 hunks)engine/runtime/action-processor/src/lib.rs(1 hunks)engine/runtime/action-processor/src/mapping.rs(2 hunks)engine/runtime/action-processor/src/wasm.rs(1 hunks)engine/runtime/action-processor/src/wasm/errors.rs(1 hunks)engine/runtime/action-processor/src/wasm/mapping.rs(1 hunks)engine/runtime/action-processor/src/wasm/runtime_executor.rs(1 hunks)engine/runtime/examples/plateau/testdata/workflow/wasm_runtime_executor.yml(1 hunks)engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/expect.json(1 hunks)engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py(1 hunks)engine/runtime/tests/fixture/workflow/wasm/runtime_executor/attribute_python.yaml(1 hunks)engine/runtime/tests/testcase.rs(1 hunks)engine/runtime/tests/testcase/wasm.rs(1 hunks)engine/runtime/tests/testcase/wasm/runtime_executor.rs(1 hunks)engine/schema/actions.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- engine/runtime/action-processor/src/lib.rs
- engine/runtime/action-processor/src/wasm.rs
- engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/expect.json
- engine/runtime/tests/testcase.rs
- engine/runtime/tests/testcase/wasm.rs
🔇 Additional comments (21)
engine/runtime/tests/testcase/wasm/runtime_executor.rs (2)
5-5: Verify the existence of test data files
Let's verify that the test data files exist at the specified paths.
✅ Verification successful
The previous search might have failed due to pattern matching issues. Let's try a broader search to locate the test files and understand the directory structure better.
Test data files exist and are properly structured
The test files for wasm/runtime_executor/attribute_python exist at the correct location:
engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.pyengine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/expect.json
Both files contain valid and matching content - the Python script modifies attributes by appending "_modified" to string values, and the expect.json contains the expected modified output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the existence of test data files
# Expected: Find the test data and expect.json files
# Search for the test data directory and files
fd -t f . -p "*wasm/runtime_executor/attribute_python*"
# Check content of expect.json if it exists
fd -t f expect.json -x cat {}
Length of output: 1878
Script:
#!/bin/bash
# Let's try a different approach to find the test files
# Search for files in the wasm directory
fd -t f . wasm
# Also check for any Python files that might be related
fd -t f . -e py
Length of output: 213
Script:
#!/bin/bash
# Now that we found the Python file, let's check its directory for the expect.json
ls -la engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/
# Also check the content of the Python file
cat engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py
Length of output: 1204
Script:
#!/bin/bash
# Let's check the content of expect.json in this directory
cat engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/expect.json
Length of output: 179
1-2: Verify the helper function implementation
The test relies on a helper function execute_with_test_assert. Let's verify its implementation to ensure proper test setup and teardown.
✅ Verification successful
Let's gather more information about the helper function implementation and test data.
Based on the gathered information, I can now generate the final response:
Helper function is well-tested and follows established patterns
The execute_with_test_assert helper function is part of a robust test framework that:
- Properly sets up test environment with temporary directories
- Handles test fixtures and assertions consistently
- Is used across multiple test modules (bulk_renamer, runtime_executor, etc.)
- Follows the same pattern as other integration tests in the codebase
The implementation shows proper error handling and resource cleanup:
pub(crate) fn execute_with_test_assert(test_id: &str, assert_file: &str) {
let tempdir = execute(test_id, vec![]).unwrap();
let storage_resolver = Arc::new(StorageResolver::new());
// ...
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and analyze the helper function implementation
# Expected: Find the helper function that handles test execution and assertions
# Search for the helper function definition
ast-grep --pattern 'fn execute_with_test_assert($$$)'
# Search for related test utilities
rg -g '*.rs' -A 5 'execute_with_test_assert'
Length of output: 8801
Script:
#!/bin/bash
# Search for the execute function and related test utilities
rg -g '*.rs' -A 10 'fn execute\(' engine/runtime/tests/
# Check if test data exists
fd -g 'expect.json' 'wasm/runtime_executor'
# Look for test fixtures implementation
ast-grep --pattern 'struct Fixtures {
$$$
}'
# Check for other test cases in the same directory
fd -g '*.rs' . engine/runtime/tests/testcase/wasm/
Length of output: 22491
engine/runtime/action-processor/src/wasm/errors.rs (2)
1-13: LGTM! The error handling structure is well-organized.
The use of thiserror and the separation of concerns between factory and runtime errors is good. The visibility modifiers are appropriate for the module structure.
3-3: Consider removing #[allow(dead_code)] attributes.
These attributes suggest the code is not yet used. If this is work in progress, consider adding a TODO comment explaining when these will be used. If they are already being used, these attributes can be removed.
Also applies to: 12-12
engine/runtime/action-processor/src/wasm/mapping.rs (2)
1-6: LGTM! Clean and appropriate imports.
The imports are well-organized and the use of once_cell::Lazy ensures thread-safe lazy initialization.
8-15: Consider future extensibility of the factory mappings.
While the current implementation with a single WasmRuntimeExecutorFactory is clean, consider these architectural improvements:
- Add a method to dynamically register new factories at runtime
- Consider using a builder pattern for more flexible initialization
- Add support for factory versioning
Let's verify that factory names are unique across the codebase to prevent future naming conflicts:
engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py (3)
1-3: LGTM! Minimal and appropriate imports.
The script uses only standard library imports, which is ideal for WebAssembly compilation.
29-30: LGTM! Proper script entry point.
The implementation follows Python best practices for script execution.
1-30: Verify WebAssembly compilation compatibility.
Please ensure that all Python features used in this script are supported by py2wasm compiler:
- Standard library imports (sys, json)
- Dictionary operations
- String operations
- Exception handling
- File I/O (sys.stdout)
✅ Verification successful
Based on the search results, I found that the project does use py2wasm for compiling Python scripts to WebAssembly, and it's documented in the engine/README.md. Let me verify the specific Python features used in the script against the project's codebase to see if there are similar patterns in use.
Let me try one more search to find any WASM-related configuration or test files that might indicate supported features.
Python script is compatible with WebAssembly compilation
The script uses standard Python features that are supported by the project's WebAssembly implementation:
- The exact same Python script is already being used as a test fixture in the project (
engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py) - The codebase includes test cases that successfully compile and execute this Python script (
engine/runtime/tests/testcase/wasm/runtime_executor.rs) - All used features (sys, json, dictionaries, string operations, exception handling, stdout) are working in the existing test environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if py2wasm is installed and verify its Python feature support
# Check py2wasm installation
which py2wasm || echo "py2wasm not found"
# Look for documentation about supported features
fd -t f "README|FEATURES|COMPATIBILITY" -e md -e txt | xargs rg -i "python|feature|support" -A 5
Length of output: 4589
Script:
#!/bin/bash
# Search for similar Python patterns in WASM-related test files and implementations
# Look for similar imports and operations in other Python files under wasm directory
fd -t f -e py . engine/runtime/tests/fixture/testdata/wasm | xargs cat
# Search for any WASM-related test files that might show supported features
rg -g "*.rs" "wasm.*test" -A 5 engine/
Length of output: 911
Script:
#!/bin/bash
# Search for WASM-related files and configurations
rg -g "!*.py" -i "python.*wasm|py2wasm" -A 5 engine/
# Look for test files or documentation specifically about Python WASM support
fd -t f . engine/runtime | xargs rg -i "python.*attribute|wasm.*python" -A 5
Length of output: 7392
engine/runtime/action-processor/src/mapping.rs (2)
7-8: LGTM! Clean import addition.
The wasm module import is cleanly integrated with the existing imports, maintaining consistent formatting.
26-26: Verify wasm mapping structure matches other processors.
The wasm mappings are integrated at the root level, but the PR objectives mention this is specifically for attributes. Let's verify the structure matches other processor modules.
✅ Verification successful
The wasm mapping structure matches the pattern used across all processor modules
The wasm mapping follows the same consistent structure as all other processor modules:
- Uses the same
ACTION_FACTORY_MAPPINGSstatic Lazy HashMap - Follows identical initialization pattern with factory vectors
- Uses the same mapping transformation to create name-to-NodeKind pairs
- Matches the single-processor pattern seen in the file module, while other modules like attribute, feature, geometry, and xml have multiple processors
The integration is correctly structured and aligns with the established patterns across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare wasm mapping structure with other processor modules
# Check the wasm mapping structure
echo "Checking wasm mapping structure:"
ast-grep --pattern 'pub static ACTION_FACTORY_MAPPINGS' -f 'engine/runtime/action-processor/src/wasm/mapping.rs' -A 10
# Compare with attribute mapping for consistency
echo "\nComparing with attribute mapping structure:"
ast-grep --pattern 'pub static ACTION_FACTORY_MAPPINGS' -f 'engine/runtime/action-processor/src/attribute/mapping.rs' -A 10
Length of output: 774
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to examine the mapping structures
echo "Checking wasm mapping structure:"
rg -A 10 "ACTION_FACTORY_MAPPINGS" "engine/runtime/action-processor/src/wasm/mapping.rs"
echo -e "\nChecking attribute mapping structure:"
rg -A 10 "ACTION_FACTORY_MAPPINGS" "engine/runtime/action-processor/src/attribute/mapping.rs"
# Also check if there are any other mapping files for comparison
echo -e "\nChecking for other mapping files:"
fd "mapping.rs" "engine/runtime/action-processor/src"
Length of output: 1739
Script:
#!/bin/bash
# Let's check the structure of other mapping files for a complete comparison
echo "Checking feature mapping structure:"
rg -A 10 "ACTION_FACTORY_MAPPINGS" "engine/runtime/action-processor/src/feature/mapping.rs"
echo -e "\nChecking file mapping structure:"
rg -A 10 "ACTION_FACTORY_MAPPINGS" "engine/runtime/action-processor/src/file/mapping.rs"
echo -e "\nChecking geometry mapping structure:"
rg -A 10 "ACTION_FACTORY_MAPPINGS" "engine/runtime/action-processor/src/geometry/mapping.rs"
echo -e "\nChecking xml mapping structure:"
rg -A 10 "ACTION_FACTORY_MAPPINGS" "engine/runtime/action-processor/src/xml/mapping.rs"
Length of output: 2559
engine/runtime/action-processor/Cargo.toml (1)
50-51: Verify workspace-level dependencies and security implications.
The addition of WebAssembly runtime dependencies looks good, but please ensure:
- These dependencies are properly declared in the workspace's root
Cargo.tomlwith appropriate version constraints. - The WebAssembly execution is properly sandboxed using Wasmer's security features.
Let's verify the workspace dependencies:
✅ Verification successful
Dependencies are properly configured in the workspace
The wasmer and wasmer-wasix dependencies are correctly defined in the workspace root (engine/Cargo.toml) with specific version constraints:
- wasmer = "5.0.1"
- wasmer-wasix = "0.31.0"
This validates that the workspace-level version management is properly set up for the WebAssembly runtime dependencies used in the action processor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if wasmer dependencies are properly declared in the workspace
# Expected: Find wasmer dependencies in the root Cargo.toml
# Check root Cargo.toml for wasmer dependencies
rg -A 2 'wasmer = .*' ./Cargo.toml
rg -A 2 'wasmer-wasix = .*' ./Cargo.toml
Length of output: 185
Script:
#!/bin/bash
# Let's first locate the workspace root Cargo.toml file
fd -t f "Cargo.toml" -x echo "Found: {}"
# Then check its content for wasmer dependencies
fd -t f "Cargo.toml" -x cat {} | grep -A 2 "wasmer"
Length of output: 1313
Script:
#!/bin/bash
# Let's check the engine/Cargo.toml which is likely the workspace root
cat ./engine/Cargo.toml | grep -A 5 "\[workspace\]"
# Also check for wasmer in workspace dependencies section
cat ./engine/Cargo.toml | grep -A 5 "\[workspace.dependencies\]"
Length of output: 586
Script:
#!/bin/bash
# Let's get the complete content of engine/Cargo.toml to check all workspace dependencies
cat ./engine/Cargo.toml
Length of output: 6091
engine/runtime/examples/plateau/testdata/workflow/wasm_runtime_executor.yml (2)
14-20: Verify the custom syntax for attribute creation
The #{ syntax for attribute definition appears unusual. Please verify that this is the correct syntax supported by the FeatureCreator action.
✅ Verification successful
The #{ syntax is the correct format for FeatureCreator attributes
Based on the search results, the #{ syntax is consistently used across multiple test and example files in the codebase for defining attributes in FeatureCreator actions. This pattern appears in:
- Test fixtures (
engine/runtime/tests/fixture/workflow/...) - Example workflows (
engine/runtime/examples/plateau/testdata/workflow/...)
The syntax is standardized and matches the implementation under review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other examples of FeatureCreator syntax
rg -A 5 "FeatureCreator" --type yaml
Length of output: 22361
37-48: Verify port compatibility between nodes
The workflow uses default ports for all connections. Please verify:
- The output format of FeatureCreator matches the input requirements of WasmRuntimeExecutor
- The output format of WasmRuntimeExecutor matches the input requirements of EchoSink
✅ Verification successful
Based on the documentation and workflow files, I can now generate the final response:
Port compatibility is verified and correct
The workflow connections are compatible:
- FeatureCreator -> WasmRuntimeExecutor:
- FeatureCreator outputs feature objects with attributes on the default port
- WasmRuntimeExecutor accepts features on the default port and processes their attributes (processorType: "Attribute")
- WasmRuntimeExecutor -> EchoSink:
- WasmRuntimeExecutor outputs processed features on the default port
- EchoSink accepts any feature input on its default port
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for port definitions and data format specifications
ast-grep --pattern 'action: $_(FeatureCreator|WasmRuntimeExecutor|EchoSink)'
Length of output: 78
Script:
#!/bin/bash
# Search for node definitions in the workflow file
rg -A 10 "nodes:" engine/runtime/examples/plateau/testdata/workflow/wasm_runtime_executor.yml
# Search for any documentation or type definitions related to these components
fd -e md -e yaml -e yml . | xargs rg -l "FeatureCreator|WasmRuntimeExecutor|EchoSink"
Length of output: 2749
Script:
#!/bin/bash
# Let's check the documentation for these components
cat engine/docs/mdbook/src/action.md
# Check example workflows that use these components
cat engine/runtime/tests/fixture/workflow/wasm/runtime_executor/attribute_python.yaml
# Check the actual workflow file completely
cat engine/runtime/examples/plateau/testdata/workflow/wasm_runtime_executor.yml
Length of output: 48833
engine/containers/worker/Dockerfile (2)
52-55: Consider version pinning and security implications.
While the Python installation is correctly grouped in a single RUN command, consider:
- Pin specific versions of Python packages for reproducibility and security.
- Evaluate if Python needs to be installed in the final image or if compilation can be done in a separate stage.
Let's verify if Python is actually needed in the runtime:
✅ Verification successful
Python installation in the final image is necessary
Based on the evidence from the codebase:
- The runtime executor (
engine/runtime/action-processor/src/wasm/runtime_executor.rs) directly invokespy2wasmcommand to compile Python code to WebAssembly at runtime. - Test fixtures and examples show that Python source files are processed during execution, not just at build time.
- The multi-stage Dockerfile correctly separates the build environment from the runtime environment, installing only the necessary Python packages in the final stage.
While version pinning could improve reproducibility, the current setup is architecturally sound as Python and py2wasm are required runtime dependencies for the WebAssembly compilation feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Python is used at runtime or only during compilation
# Look for Python imports or runtime usage in the Rust code
rg -g '*.rs' -A 5 'python|py2wasm'
Length of output: 2240
Script:
#!/bin/bash
# Let's check if there are any Python files in the codebase that are used at runtime
fd -e py -x file {}
# Also check if there are any Python-related environment variables or configurations
rg -g '*.{yaml,yml,json,toml}' 'python|py2wasm'
# Check the Dockerfile more thoroughly to understand the build stages
cat engine/containers/worker/Dockerfile
Length of output: 2683
58-58: 🛠️ Refactor suggestion
Optimize layer and pin package versions.
The pip commands can be optimized and made more secure:
- Consider combining with the previous RUN command to reduce layers
- Pin specific versions of pip and py2wasm
Consider this optimization:
RUN \
--mount=type=cache,target=/var/lib/apt,sharing=locked \
--mount=type=cache,target=/var/cache/apt,sharing=locked \
apt-get update && \
apt-get install -y \
ca-certificates \
libxml2 \
python3.11 \
python3.11-distutils \
python3-pip && \
- rm -rf /var/lib/apt/lists/*
-
-RUN python3.11 -m pip install --upgrade pip && pip install py2wasm
+ rm -rf /var/lib/apt/lists/* && \
+ python3.11 -m pip install --upgrade "pip==23.3.1" && \
+ pip install "py2wasm==0.1.0"Let's check for the latest stable versions:
✅ Verification successful
Pin py2wasm to version 2.6.3 for consistent builds
The package is actively used in the codebase for compiling Python scripts to WebAssembly, particularly in the worker and CLI containers. Let's update the suggestion to use the latest stable version:
RUN \
--mount=type=cache,target=/var/lib/apt,sharing=locked \
--mount=type=cache,target=/var/cache/apt,sharing=locked \
apt-get update && \
apt-get install -y \
ca-certificates \
libxml2 \
python3.11 \
python3.11-distutils \
python3-pip && \
- rm -rf /var/lib/apt/lists/*
-
-RUN python3.11 -m pip install --upgrade pip && pip install py2wasm
+ rm -rf /var/lib/apt/lists/* && \
+ python3.11 -m pip install --upgrade "pip==23.3.1" && \
+ pip install "py2wasm==2.6.3"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest versions of pip and py2wasm
curl -s https://pypi.org/pypi/py2wasm/json | jq '.info.version'
Length of output: 71
Script:
#!/bin/bash
# Check if py2wasm is used in the codebase and how
rg -l "py2wasm" --type-add 'docker:*.{dockerfile,Dockerfile}' -tdocker
rg "py2wasm" -A 5 -B 5
Length of output: 5402
engine/runtime/tests/fixture/workflow/wasm/runtime_executor/attribute_python.yaml (2)
43-54: LGTM: Edge definitions are clear and appropriate
The workflow structure is well-defined with proper connections between nodes using default ports.
5-6: Verify handling of empty outputFilePath
The outputFilePath parameter is defined but empty. Since this parameter is used by the FileWriter action (line 41), ensure that the workflow properly handles the case when the environment variable is not set.
✅ Verification successful
Let me gather more information about how the outputFilePath is handled in the codebase.
Let me try a different approach to find FileWriter and output path handling.
Based on the search results, I can see that the FileWriter implementation in engine/runtime/action-sink/src/file/writer.rs properly handles the output path. The code shows that:
- The output path is properly parsed into a URI (
Uri::from_str(path.as_str())?) - The storage system handles the path creation and writing (
put_sync(output.path().as_path(), ...)) - Error handling is in place for file writing operations
Empty outputFilePath is handled by environment variable substitution
The empty outputFilePath in the YAML file is intentional as it's meant to be populated by an environment variable during test execution, which is confirmed by the test helper code in engine/runtime/tests/helper.rs that sets it to result.json in the test folder.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's error handling for missing outputFilePath in the codebase
rg -A 5 "outputFilePath" --type rust
Length of output: 407
Script:
#!/bin/bash
# Search for FileWriter action implementation and its handling of output path
ast-grep --pattern 'struct FileWriter'
rg -B 2 -A 10 "FileWriter.*impl"
Length of output: 75
Script:
#!/bin/bash
# Search for FileWriter in yaml files to understand its usage pattern
rg -A 10 "FileWriter:" --type yaml
# Search for any file handling or path validation code
rg -A 5 "file_path|filepath|output.*path" --type rust
Length of output: 33180
.github/workflows/ci_engine.yml (2)
49-52: LGTM! Python setup looks good.
The Python installation step is correctly configured using the official GitHub Action with a specific version.
49-56: Verify integration with subsequent steps.
The Python environment setup looks good, but we should ensure it integrates well with the test execution.
✅ Verification successful
Python 3.11 and py2wasm setup is correctly integrated with subsequent steps
The verification confirms that Python 3.11 and py2wasm are properly integrated:
- Both worker and CLI Dockerfiles correctly install Python 3.11 and py2wasm
- The runtime executor (
runtime_executor.rs) has proper integration with py2wasm for compiling Python scripts to WASM - Test fixtures demonstrate the working integration with a sample Python script
- The README documents this as an optional requirement for WASM functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any tests or build scripts explicitly require Python 3.11
# Look for Python version requirements in test files and configs
rg -l "python.*3\.11|py2wasm"
# Look for potential Python-related test files
fd -e py -e pyw
# Look for potential WASM test files
fd -e wasm
Length of output: 296
Script:
#!/bin/bash
# Check content of relevant files to understand Python/WASM integration
cat engine/README.md
cat engine/containers/worker/Dockerfile
cat engine/containers/cli/Dockerfile
cat engine/runtime/action-processor/src/wasm/runtime_executor.rs
cat engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py
Length of output: 18998
engine/Cargo.toml (1)
175-176: Consider upgrading wasmer-wasix and document security measures.
A few recommendations regarding the WebAssembly runtime dependencies:
- Consider upgrading
wasmer-wasixto version 0.32.0 for the latest features and security patches. - Document the WebAssembly runtime configuration, specifically:
- Memory limits
- CPU time limits
- Filesystem access permissions
- Network access restrictions
These security measures are crucial when executing untrusted WebAssembly code.
Let's verify the latest versions and any security advisories:
| fn test_attribute_python() { | ||
| execute_with_test_assert("wasm/runtime_executor/attribute_python", "expect.json"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases
While this test covers the basic attribute processing scenario, consider adding the following test cases for better coverage:
- Error handling for invalid Python syntax
- Edge cases with empty or malformed attributes
- Memory/resource limits in the Wasm runtime
- Different Python script complexities
- Concurrent execution scenarios
Here's a suggested expansion of the test module:
#[test]
fn test_attribute_python() {
execute_with_test_assert("wasm/runtime_executor/attribute_python", "expect.json");
}
#[test]
fn test_attribute_python_invalid_syntax() {
// Test handling of invalid Python code
execute_with_test_assert("wasm/runtime_executor/attribute_python_invalid", "expect_error.json");
}
#[test]
fn test_attribute_python_empty() {
// Test handling of empty attributes
execute_with_test_assert("wasm/runtime_executor/attribute_python_empty", "expect_empty.json");
}
#[test]
fn test_attribute_python_complex() {
// Test with complex Python operations
execute_with_test_assert("wasm/runtime_executor/attribute_python_complex", "expect_complex.json");
}| let factories: Vec<Box<dyn ProcessorFactory>> = | ||
| vec![Box::<WasmRuntimeExecutorFactory>::default()]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for factory initialization.
The current implementation using Box::<WasmRuntimeExecutorFactory>::default() could panic if initialization fails. Consider:
- Using a Result type for graceful error handling
- Adding validation for the factory initialization
- Implementing proper error propagation
Here's a suggested improvement:
- let factories: Vec<Box<dyn ProcessorFactory>> =
- vec![Box::<WasmRuntimeExecutorFactory>::default()];
+ let factories: Vec<Box<dyn ProcessorFactory>> = vec![
+ WasmRuntimeExecutorFactory::new()
+ .map(Box::new)
+ .expect("Failed to initialize WasmRuntimeExecutorFactory")
+ ];Committable suggestion skipped: line range outside the PR's diff.
| def process_attributes(attributes): | ||
| updated_attributes = {} | ||
| for key, value in attributes.items(): | ||
| if isinstance(value, dict) and "String" in value: | ||
| updated_attributes[key] = {"String": value["String"] + "_modified"} | ||
| else: | ||
| updated_attributes[key] = value | ||
| return updated_attributes |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and input validation for better type safety.
Consider these improvements for better maintainability and type safety, especially important for WebAssembly compilation:
-def process_attributes(attributes):
+from typing import Dict, Any
+
+def process_attributes(attributes: Dict[str, Any]) -> Dict[str, Any]:
+ """Process attributes by appending '_modified' to string values.
+
+ Args:
+ attributes: Dictionary of attribute key-value pairs
+
+ Returns:
+ Dictionary with modified string values
+ """
+ if not isinstance(attributes, dict):
+ raise TypeError("attributes must be a dictionary")
+
updated_attributes = {}
for key, value in attributes.items():
if isinstance(value, dict) and "String" in value:
+ if not isinstance(value["String"], str):
+ raise TypeError(f"Value for key '{key}' must be a string")
updated_attributes[key] = {"String": value["String"] + "_modified"}
else:
updated_attributes[key] = value
return updated_attributes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def process_attributes(attributes): | |
| updated_attributes = {} | |
| for key, value in attributes.items(): | |
| if isinstance(value, dict) and "String" in value: | |
| updated_attributes[key] = {"String": value["String"] + "_modified"} | |
| else: | |
| updated_attributes[key] = value | |
| return updated_attributes | |
| from typing import Dict, Any | |
| def process_attributes(attributes: Dict[str, Any]) -> Dict[str, Any]: | |
| """Process attributes by appending '_modified' to string values. | |
| Args: | |
| attributes: Dictionary of attribute key-value pairs | |
| Returns: | |
| Dictionary with modified string values | |
| """ | |
| if not isinstance(attributes, dict): | |
| raise TypeError("attributes must be a dictionary") | |
| updated_attributes = {} | |
| for key, value in attributes.items(): | |
| if isinstance(value, dict) and "String" in value: | |
| if not isinstance(value["String"], str): | |
| raise TypeError(f"Value for key '{key}' must be a string") | |
| updated_attributes[key] = {"String": value["String"] + "_modified"} | |
| else: | |
| updated_attributes[key] = value | |
| return updated_attributes |
| def main(): | ||
| try: | ||
| input_data = sys.argv[1] | ||
| parsed_data = json.loads(input_data) | ||
|
|
||
| updated_attributes = process_attributes(parsed_data) | ||
|
|
||
| output = {"status": "success", "attributes": updated_attributes, "error": None} | ||
| except Exception as e: | ||
| output = {"status": "error", "attributes": None, "error": str(e)} | ||
|
|
||
| print(json.dumps(output), file=sys.stdout) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add input validation.
The current error handling could be more specific and informative:
def main():
try:
+ if len(sys.argv) != 2:
+ raise ValueError("Expected exactly one argument containing JSON data")
+
input_data = sys.argv[1]
- parsed_data = json.loads(input_data)
+ try:
+ parsed_data = json.loads(input_data)
+ except json.JSONDecodeError as e:
+ raise ValueError(f"Invalid JSON input: {str(e)}")
+
+ if not isinstance(parsed_data, dict):
+ raise TypeError("JSON input must be an object")
updated_attributes = process_attributes(parsed_data)
output = {"status": "success", "attributes": updated_attributes, "error": None}
+ except ValueError as e:
+ output = {"status": "error", "attributes": None, "error": f"Input error: {str(e)}"}
+ except TypeError as e:
+ output = {"status": "error", "attributes": None, "error": f"Type error: {str(e)}"}
except Exception as e:
- output = {"status": "error", "attributes": None, "error": str(e)}
+ output = {"status": "error", "attributes": None, "error": f"Unexpected error: {str(e)}"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def main(): | |
| try: | |
| input_data = sys.argv[1] | |
| parsed_data = json.loads(input_data) | |
| updated_attributes = process_attributes(parsed_data) | |
| output = {"status": "success", "attributes": updated_attributes, "error": None} | |
| except Exception as e: | |
| output = {"status": "error", "attributes": None, "error": str(e)} | |
| print(json.dumps(output), file=sys.stdout) | |
| def main(): | |
| try: | |
| if len(sys.argv) != 2: | |
| raise ValueError("Expected exactly one argument containing JSON data") | |
| input_data = sys.argv[1] | |
| try: | |
| parsed_data = json.loads(input_data) | |
| except json.JSONDecodeError as e: | |
| raise ValueError(f"Invalid JSON input: {str(e)}") | |
| if not isinstance(parsed_data, dict): | |
| raise TypeError("JSON input must be an object") | |
| updated_attributes = process_attributes(parsed_data) | |
| output = {"status": "success", "attributes": updated_attributes, "error": None} | |
| except ValueError as e: | |
| output = {"status": "error", "attributes": None, "error": f"Input error: {str(e)}"} | |
| except TypeError as e: | |
| output = {"status": "error", "attributes": None, "error": f"Type error: {str(e)}"} | |
| except Exception as e: | |
| output = {"status": "error", "attributes": None, "error": f"Unexpected error: {str(e)}"} | |
| print(json.dumps(output), file=sys.stdout) |
| sourceCodeFilePath: "runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py" | ||
| wasmFilePath: "runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.wasm" |
There was a problem hiding this comment.
💡 Codebase verification
Missing WASM file and documentation needed
The verification reveals:
- Python source file exists at the correct path but the corresponding WASM file is missing
- No documentation exists about Python requirements or WASM compilation steps
- The workflow configuration references paths that are partially incorrect
Action items:
- Add the missing WASM file at
runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.wasm - Document Python version requirements and WASM compilation steps in README.md
- Update the paths in the workflow file to match the actual file structure (
runtime/tests/fixture/...instead of justruntime/tests/...)
🔗 Analysis chain
Validate file paths and add documentation
The relative paths to source and WASM files need validation:
- Ensure these paths are correct relative to the workflow execution context
- Consider documenting Python version requirements and WASM compilation steps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of referenced files
fd "sample.py|sample.wasm" --type f
Length of output: 124
Script:
#!/bin/bash
# Check if the wasm file exists in any form (compressed or uncompressed)
fd "sample.wasm" --type f --hidden
# Check the actual paths in the workflow file to ensure we're looking at the right paths
cat engine/runtime/examples/plateau/testdata/workflow/wasm_runtime_executor.yml
# Look for any documentation about WASM compilation or Python requirements
rg -i "python|wasm" README.md || echo "No matches in README.md"
Length of output: 1821
| rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN python3.11 -m pip install --upgrade pip && pip install py2wasm |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security and maintainability of pip installations.
The current pip installation could be improved for security and maintainability:
- Pin py2wasm version for reproducible builds
- Run pip as non-root user
- Clean pip cache to reduce image size
-RUN python3.11 -m pip install --upgrade pip && pip install py2wasm
+RUN python3.11 -m pip install --no-cache-dir pip==23.2.1 && \
+ python3.11 -m pip install --no-cache-dir py2wasm==0.1.0 && \
+ rm -rf /root/.cache/pip/*📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN python3.11 -m pip install --upgrade pip && pip install py2wasm | |
| RUN python3.11 -m pip install --no-cache-dir pip==23.2.1 && \ | |
| python3.11 -m pip install --no-cache-dir py2wasm==0.1.0 && \ | |
| rm -rf /root/.cache/pip/* |
| libxml2 \ | ||
| python3.11 \ | ||
| python3.11-distutils \ | ||
| python3-pip && \ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider version pinning Python packages for reproducible builds.
While adding Python support is correct, consider:
- Pin specific versions of Python packages for reproducibility
- Minimize the final image size by combining RUN commands
- libxml2 \
- python3.11 \
- python3.11-distutils \
- python3-pip && \
+ libxml2=2.9.* \
+ python3.11=3.11.* \
+ python3.11-distutils=3.11.* \
+ python3-pip=23.* && \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| libxml2 \ | |
| python3.11 \ | |
| python3.11-distutils \ | |
| python3-pip && \ | |
| libxml2=2.9.* \ | |
| python3.11=3.11.* \ | |
| python3.11-distutils=3.11.* \ | |
| python3-pip=23.* && \ |
| output: | | ||
| env.get("outputFilePath") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default value and validation for output path
The output path relies on an environment variable without a default value. Consider adding a default value and path validation.
output: |
- env.get("outputFilePath")
+ env.get("outputFilePath", "output/default.json") |> path.validate()Committable suggestion skipped: line range outside the PR's diff.
| sourceCodeFilePath: "fixture/testdata/wasm/runtime_executor/attribute_python/sample.py" | ||
| wasmFilePath: "fixture/testdata/wasm/runtime_executor/attribute_python/sample.wasm" |
There was a problem hiding this comment.
💡 Codebase verification
Missing WASM file in the referenced location
The Python source file exists at the correct location with proper read permissions, but the WASM file is missing. The workflow configuration expects both files at:
- ✅
fixture/testdata/wasm/runtime_executor/attribute_python/sample.py(exists) - ❌
fixture/testdata/wasm/runtime_executor/attribute_python/sample.wasm(missing)
This could lead to runtime failures as the WebAssembly executor requires both files to function properly.
🔗 Analysis chain
Verify existence and permissions of referenced files
The workflow references external Python source and WASM files. Ensure these files exist and have proper permissions.
The initial check shows that while the Python file exists, the WASM file wasn't found in the expected location. Let's verify the actual paths configured in the YAML and search for the WASM file in nearby directories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced files exist
fd -t f "sample.py|sample.wasm" -p "fixture/testdata/wasm/runtime_executor/attribute_python"
# Check file permissions
find . -path "*fixture/testdata/wasm/runtime_executor/attribute_python/*" -type f -exec ls -l {} \;
Length of output: 615
Script:
#!/bin/bash
# Check if the WASM file exists in any nearby directories
fd -t f "sample.wasm" ./engine/runtime/tests/fixture/testdata/wasm/runtime_executor
# Check the actual path in the YAML file
cat ./engine/runtime/tests/fixture/workflow/wasm/runtime_executor/attribute_python.yaml
Length of output: 1884
engine/runtime/tests/fixture/testdata/wasm/runtime_executor/attribute_python/sample.py
Show resolved
Hide resolved
| } | ||
|
|
||
| fn execute_wasm_module(&self, input: &str) -> super::errors::Result<String> { | ||
| let wasm_bytes = std::fs::read(&self.params.wasm_file_path).map_err(|e| { |
There was a problem hiding this comment.
Important
There is a process to get the wasm binary from outside each time, but please consider if it is possible to cache it.
|
@soma00333 |
|
I have fixed here to handle the review |
soma00333
force-pushed
the
feat/engine/wasm-runtime-executor
branch
from
54f39e7 to
ebb119b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
engine/runtime/action-processor/src/wasm/runtime_executor.rs (2)
100-131: Provide error handling for unsupported programming languagesCurrently, the
compile_to_wasmmethod only handlesProgrammingLanguage::Python. If other languages are added in the future, the method will silently ignore them. Consider adding an else clause to handle unsupported languages and return an appropriate error.Apply this diff to handle unsupported languages:
match params.programming_language { ProgrammingLanguage::Python => { // Existing implementation }, + _ => { + return Err(WasmProcessorError::RuntimeExecutorFactory(format!( + "Unsupported programming language: {:?}", + params.programming_language + ))); + } }
220-235: Capture and handlestderroutput from the Wasm module executionCurrently, only
stdoutis being captured from the Wasm module execution. If the module writes errors or important messages tostderr, they will not be visible. Consider capturingstderras well to improve error reporting and debugging.Apply this diff to capture
stderroutput:let (stdout_tx, mut stdout_rx) = Pipe::channel(); +let (stderr_tx, mut stderr_rx) = Pipe::channel(); WasiEnv::builder(program_name) .args([input]) .stdout(Box::new(stdout_tx)) + .stderr(Box::new(stderr_tx)) .run_with_store(module, &mut store) .map_err(|e| { WasmProcessorError::RuntimeExecutor(format!("Failed to execute module: {}", e)) })?; // Read stdout let mut output = String::new(); stdout_rx.read_to_string(&mut output).map_err(|e| { WasmProcessorError::RuntimeExecutor(format!( "Failed to read stdout from Wasm module: {}", e )) })?; +// Read stderr +let mut error_output = String::new(); +stderr_rx.read_to_string(&mut error_output).map_err(|e| { + WasmProcessorError::RuntimeExecutor(format!( + "Failed to read stderr from Wasm module: {}", + e + )) +})?; +// Incorporate `error_output` into error handling if necessary
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
engine/Cargo.lockis excluded by!**/*.lock,!**/*.lockengine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (3)
engine/runtime/action-processor/Cargo.toml(1 hunks)engine/runtime/action-processor/src/wasm/runtime_executor.rs(1 hunks)engine/schema/actions.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/runtime/action-processor/Cargo.toml
- engine/schema/actions.json
🔇 Additional comments (1)
engine/runtime/action-processor/src/wasm/runtime_executor.rs (1)
191-193: Verify attribute serialization for potential edge cases
When serializing attributes to JSON, certain values might not serialize properly, leading to errors. Ensure that all attribute values are serializable and handle any serialization errors gracefully.
Run the following script to check for attributes that may not serialize properly:
✅ Verification successful
Serialization implementation is robust and handles all edge cases
The AttributeValue enum has a complete and safe serialization implementation:
- It derives
Serializeand has proper implementations for all variants - All variant types (
bool,Number,String,DateTime,Vec,Bytes,HashMap) are standard serializable types - It has comprehensive
From/Intoimplementations forserde_json::Valuethat handle all cases - Error handling is in place through the
WasmProcessorErrorwrapper - The implementation is well-tested as shown by the test suite
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find attribute values that may not serialize to JSON.
# Search for AttributeValue instances that might cause serialization issues
fd -e rs -x ast-grep --pattern 'AttributeValue::{{$_}}({{$$$}})' {}
# Expected: All AttributeValues should be in a serializable format.
Length of output: 71
Script:
#!/bin/bash
# Let's try a different approach to find attribute-related code
# First, let's find the AttributeValue enum definition
rg -p "enum AttributeValue" -A 10
# Then look for the serialize_attributes implementation
rg -p "fn serialize_attributes" -A 10
# Also check for any serde serialization implementations
rg -p "impl.*Serialize.*for AttributeValue" -A 10
Length of output: 878
Script:
#!/bin/bash
# Let's check for the serde derive or manual implementation for AttributeValue
rg -p "derive.*Serialize.*AttributeValue|impl.*Serialize.*for AttributeValue" -B 2 -A 10
# Also check if there are any tests related to attribute serialization
rg -p "test.*serialize.*attribute" -A 10
# Look for any error handling patterns in attribute serialization
rg "WasmProcessorError::.*serialize" -A 2
Length of output: 177
Script:
#!/bin/bash
# Let's check the entire attribute.rs file to understand the complete implementation
cat engine/runtime/types/src/attribute.rs
# Also check for any custom serialization implementations in the same file
rg -p "#\[derive\(.*Serialize.*\)\]" engine/runtime/types/src/attribute.rs -B 2 -A 2
Length of output: 18929
| let module = Module::new(&store, &self.wasm_binary).map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutor(format!("Failed to compile module: {}", e)) | ||
| })?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set resource limits on Wasm module execution
To prevent resource exhaustion or infinite loops when executing untrusted Wasm modules, consider setting limits on memory usage and execution time.
Apply this diff to set resource limits:
let mut store = Store::default();
+store.set_limits(wasmer::StoreLimitsBuilder::new()
+ .memory_pages(1024) // Limit memory usage to 64MB (64KB per page)
+ .instances(10) // Limit the number of instances
+ .build());
let module = Module::new(&store, &self.wasm_binary).map_err(|e| {
WasmProcessorError::RuntimeExecutor(format!("Failed to compile module: {}", e))
})?;Note: Adjust the limits according to your application's requirements.
Committable suggestion skipped: line range outside the PR's diff.
| let py2wasm_output = std::process::Command::new("py2wasm") | ||
| .args([¶ms.source_code_file_path, "-o", temp_wasm_path_str]) | ||
| .output() | ||
| .map_err(|e| { | ||
| WasmProcessorError::RuntimeExecutorFactory(format!( | ||
| "Failed to run py2wasm: {}. Command: py2wasm {} -o {}", | ||
| e, params.source_code_file_path, temp_wasm_path_str | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Handle the case where py2wasm is not found or not installed
Currently, if py2wasm is not installed or not available in the PATH, the compilation will fail with a generic error. Consider handling the specific error when py2wasm is not found to provide a clearer message to the user.
Apply this diff to improve error handling:
let py2wasm_output = std::process::Command::new("py2wasm")
.args([¶ms.source_code_file_path, "-o", temp_wasm_path_str])
.output()
.map_err(|e| {
WasmProcessorError::RuntimeExecutorFactory(format!(
- "Failed to run py2wasm: {}. Command: py2wasm {} -o {}",
- e, params.source_code_file_path, temp_wasm_path_str
+ "Failed to run 'py2wasm'. Please ensure it is installed and in your PATH. Error: {}",
+ e
))
})?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let py2wasm_output = std::process::Command::new("py2wasm") | |
| .args([¶ms.source_code_file_path, "-o", temp_wasm_path_str]) | |
| .output() | |
| .map_err(|e| { | |
| WasmProcessorError::RuntimeExecutorFactory(format!( | |
| "Failed to run py2wasm: {}. Command: py2wasm {} -o {}", | |
| e, params.source_code_file_path, temp_wasm_path_str | |
| )) | |
| })?; | |
| let py2wasm_output = std::process::Command::new("py2wasm") | |
| .args([¶ms.source_code_file_path, "-o", temp_wasm_path_str]) | |
| .output() | |
| .map_err(|e| { | |
| WasmProcessorError::RuntimeExecutorFactory(format!( | |
| "Failed to run 'py2wasm'. Please ensure it is installed and in your PATH. Error: {}", | |
| e | |
| )) | |
| })?; |
Overview
Add a processor called
WasmRuntimeExecutorwhich compiles user's python scripts to.wasmand executes on wasm runtime.The scope is to handle processor_type:
attributeand programmig_language:python.related issue: #573
What I've done
Add crates
Add a integration test
Compiles a python script and executes on wasmer to modify the attributes.
Fix workflow of GitHub Actions
Add
python 3.11xpy2wasmdependencies for the integration testAdd a WasmRuntimeExecutor
Add a sample workflow
Compiles a python script and executes on wasmer to modify the attributes.
Other
py2wasmWhat I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
WasmRuntimeExecutor, enabling the compilation and execution of WebAssembly scripts.py2wasmpackage for enhanced functionality.Bug Fixes
Documentation
Tests