feat(engine): Add AttributeWithXML processor type to WasmRuntimeExecutor
#681
Conversation
WalkthroughThe changes in this pull request enhance the functionality of the WASM processor by introducing a new enum variant Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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.
|
…/reearth-flow into feat/engine/add-attributewithxml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
engine/runtime/examples/plateau/testdata/python/tran_xlink_checker.py (4)
75-77: Improve exception handling for better error reportingCatching all exceptions with
except Exception as ecan mask unexpected errors. Consider catching specific exceptions, such asET.ParseErrorfor XML parsing errors andjson.JSONDecodeErrorfor JSON decoding errors. This provides clearer error messages and aids in debugging.
20-33: Refactor duplicated code into a helper functionThe code blocks for processing
tran:TrafficAreaandtran:AuxiliaryTrafficAreaare similar. Refactoring them into a helper function can reduce duplication and improve maintainability.Apply this diff to refactor the code:
+def extract_surface_ids(road, area_type, lod_levels): + trf = {lod: [] for lod in lod_levels} + for x in road.findall(f'.//tran:{area_type}', XMLNS): + for lod in lod_levels: + xpath = f'./tran:lod{lod}MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]' + for p in x.findall(xpath, XMLNS): + trf[lod].append(p.get('{http://www.opengis.net/gml}id')) + return trf def process_attributes(attributes, xml_root): for road in xml_root.findall('*//tran:Road', XMLNS): gml_id = road.get('{http://www.opengis.net/gml}id') - lod2_trf = [] - lod3_trf = [] - for x in road.findall('.//tran:TrafficArea', XMLNS): - for p in x.findall('./tran:lod2MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]', XMLNS): - lod2_trf.append(p.get('{http://www.opengis.net/gml}id')) - for p in x.findall('./tran:lod3MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]', XMLNS): - lod3_trf.append(p.get('{http://www.opengis.net/gml}id')) - # similar code for AuxiliaryTrafficArea + lod_levels = ['2', '3'] + lod_trf = extract_surface_ids(road, 'TrafficArea', lod_levels) + lod_aux = extract_surface_ids(road, 'AuxiliaryTrafficArea', lod_levels) # rest of the code using lod_trf and lod_aux
15-15: Add docstrings to functions for better code documentationIncluding docstrings in the
process_attributesandmainfunctions will improve code readability and help others understand the purpose and usage of these functions.Also applies to: 64-64
13-15: Remove unnecessary blank lines to enhance readabilityThere are multiple consecutive blank lines which can be reduced to improve code clarity.
Also applies to: 62-64, 79-81
engine/runtime/action-wasm-processor/src/runtime_executor.rs (2)
254-266: Ensure proper handling of stdin write operationsWriting to
stdin_txand immediately dropping it may cause write errors in some cases. Ensure that all write operations are flushed and handled correctly before dropping the transmitter.Apply this diff to handle potential write errors:
stdin_tx.write_all(stdin.as_bytes()).map_err(|e| { WasmProcessorError::RuntimeExecutor(format!("Failed to write to stdin: {}", e)) })?; + stdin_tx.flush().map_err(|e| { + WasmProcessorError::RuntimeExecutor(format!("Failed to flush stdin: {}", e)) + })?; drop(stdin_tx);
223-231: Provide more informative error messages when accessing storageError messages in storage resolution and content retrieval could be more descriptive to aid in debugging. Include context about what operation failed.
Apply this diff to improve error messages:
let storage = ctx .storage_resolver .resolve(&uri) - .map_err(|e| WasmProcessorError::RuntimeExecutor(format!("{:?}", e)))?; + .map_err(|e| WasmProcessorError::RuntimeExecutor(format!("Failed to resolve storage for URI {}: {:?}", uri, e)))?; let content = storage .get_sync(uri.path().as_path()) - .map_err(|e| WasmProcessorError::RuntimeExecutor(format!("{:?}", e)))?; + .map_err(|e| WasmProcessorError::RuntimeExecutor(format!("Failed to get content from storage at path {}: {:?}", uri.path(), e)))?;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (4)
engine/runtime/action-wasm-processor/src/runtime_executor.rs(5 hunks)engine/runtime/examples/plateau/testdata/python/tran_xlink_checker.py(1 hunks)engine/runtime/examples/plateau/testdata/workflow/example_tran_xlink_checker_wasm.yml(1 hunks)engine/schema/actions.json(1 hunks)
🔇 Additional comments (2)
engine/runtime/examples/plateau/testdata/workflow/example_tran_xlink_checker_wasm.yml (1)
6-14: Verify the completeness of the with parameters
The with section includes parameters like cityGmlPath, cityCode, etc., which are currently empty. Ensure that all necessary parameters are provided for the workflow to function correctly.
engine/schema/actions.json (1)
2809-2810: Addition of AttributeWithXML enum value is appropriate
The inclusion of "AttributeWithXML" in the ProcessorType enum enhances the schema to support XML attribute processing as intended.
| arg_input = sys.argv[1] # attributes | ||
| parsed_arg_input = json.loads(arg_input) |
There was a problem hiding this comment.
Check for missing command-line arguments to prevent IndexError
Accessing sys.argv[1] without verifying the number of arguments can lead to an IndexError if no arguments are provided. Add a check to ensure the required arguments are present before accessing them.
Apply this diff to check for command-line arguments:
def main():
try:
+ if len(sys.argv) < 2:
+ raise ValueError("Attributes input not provided in command-line arguments.")
stdin_input = sys.stdin.read() # xml
xml_root = ET.fromstring(stdin_input)
arg_input = sys.argv[1] # attributes
# rest of the code...Committable suggestion skipped: line range outside the PR's diff.
| for road in xml_root.findall('*//tran:Road', XMLNS): | ||
| gml_id = road.get('{http://www.opengis.net/gml}id') | ||
| lod2_trf = [] | ||
| lod3_trf = [] | ||
| for x in road.findall('.//tran:TrafficArea', XMLNS): | ||
| for p in x.findall('./tran:lod2MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]', XMLNS): | ||
| lod2_trf.append(p.get('{http://www.opengis.net/gml}id')) | ||
| for p in x.findall('./tran:lod3MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]', XMLNS): | ||
| lod3_trf.append(p.get('{http://www.opengis.net/gml}id')) | ||
|
|
||
| lod2_aux = [] | ||
| lod3_aux = [] | ||
| for x in road.findall('.//tran:AuxiliaryTrafficArea', XMLNS): | ||
| for p in x.findall('./tran:lod2MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]', XMLNS): | ||
| lod2_aux.append(p.get('{http://www.opengis.net/gml}id')) | ||
| for p in x.findall('./tran:lod3MultiSurface/gml:MultiSurface/gml:surfaceMember/*[@gml:id]', XMLNS): | ||
| lod3_aux.append(p.get('{http://www.opengis.net/gml}id')) | ||
|
|
||
| lod2xlinks = [] | ||
| lod3xlinks = [] | ||
| for x in road.findall('./tran:lod2MultiSurface//*[@xlink:href]', XMLNS): | ||
| lod2xlinks.append(x.get('{http://www.w3.org/1999/xlink}href')[1:]) | ||
| for x in road.findall('./tran:lod3MultiSurface//*[@xlink:href]', XMLNS): | ||
| lod3xlinks.append(x.get('{http://www.w3.org/1999/xlink}href')[1:]) | ||
|
|
||
| lod2_unref = set(lod2_trf + lod2_aux) - set(lod2xlinks) | ||
| lod3_unref = set(lod3_trf + lod3_aux) - set(lod3xlinks) | ||
|
|
||
| if lod2_trf: | ||
| attributes.update({ | ||
| "glmId": {"String": gml_id}, | ||
| "featureType": {"String": "Road"}, | ||
| "lod": {"String": "2"}, | ||
| "unreferencedSurfaceNum": {"Number": len(lod2_unref)}, | ||
| "unreferencedIds": {"Array": [{"String": str(id)} for id in lod2_unref]}, | ||
| }) | ||
|
|
||
| if lod3_trf: | ||
| attributes.update({ | ||
| "glmId": {"String": gml_id}, | ||
| "featureType": {"String": "Road"}, | ||
| "lod": {"String": "3"}, | ||
| "unreferencedSurfaceNum": {"Number": len(lod3_unref)}, | ||
| "unreferencedIds": {"Array": [{"String": str(id)} for id in lod3_unref]}, | ||
| }) | ||
| return attributes |
There was a problem hiding this comment.
Avoid overwriting attributes when processing multiple roads
In the process_attributes function, the attributes dictionary is being updated inside a loop over roads. This could lead to earlier attribute values being overwritten if multiple tran:Road elements exist in the XML. Consider accumulating the results or processing each road separately to prevent data loss.
Apply this diff to collect attributes for all roads:
def process_attributes(attributes, xml_root):
+ results = []
for road in xml_root.findall('*//tran:Road', XMLNS):
gml_id = road.get('{http://www.opengis.net/gml}id')
lod2_trf = []
# existing code...
if lod2_trf:
- attributes.update({
+ road_attributes = {
"glmId": {"String": gml_id},
# other attributes...
- })
+ }
+ results.append(road_attributes)
# similarly for lod3_trf...
+ attributes['roads'] = results
return attributesCommittable suggestion skipped: line range outside the PR's diff.
| .get(&Attribute::new("cityGmlPath")) | ||
| .ok_or(WasmProcessorError::RuntimeExecutor( | ||
| "cityGmlPath key empty".to_string(), | ||
| ))?; |
There was a problem hiding this comment.
Properly extract string value from AttributeValue
Using city_gml_path.to_string() may not retrieve the actual string value if city_gml_path is an AttributeValue enum. Instead, match on the AttributeValue to extract the string.
Apply this diff to correctly extract the string:
let city_gml_path_value = match city_gml_path {
AttributeValue::String(s) => s.clone(),
_ => {
return Err(WasmProcessorError::RuntimeExecutor(
"cityGmlPath attribute is not a string".to_string(),
))
}
};
let uri = Uri::from_str(&city_gml_path_value).map_err(|err| {
WasmProcessorError::RuntimeExecutor(format!("cityGmlPath is not a valid uri: {}", err))
})?;Committable suggestion skipped: line range outside the PR's diff.
Overview
Add
AttributeWithXMLprocessor type toWasmRuntimeExecutorto handleTranXLinkCheckerPython script.What I've done
This pull request introduces several changes to the
engine/runtime/action-wasm-processormodule. The updates include adding support for processing XML attributes, modifying theWasmRuntimeExecutorto handle new processor types, and adding a new example workflow for XML processing.Processor and Executor Changes:
AttributeWithXMLto theProcessorTypeenum inruntime_executor.rs.process_attribute_with_xmlmethod inWasmRuntimeExecutorto handle XML attributes. This method reads XML content, parses it, and updates attributes accordingly.execute_wasm_modulemethod to accept an additionalstdinparameter for XML content and handle it appropriately.Example and Workflow Changes:
tran_xlink_checker.pyto process XML attributes and update them based on specific criteria.example_tran_xlink_checker_wasm.ymlto demonstrate the usage of theAttributeWithXMLprocessor type, including the necessary configuration and nodes.Dependency and Import Changes:
runtime_executor.rsto include necessary modules for handling URIs and XML content.What I haven't done
How I tested
Test command
Test results
lod null: WasmRuntimeExecutor
lod null: PLATEAU.TranXLinkChecker
lod2: WasmRuntimeExecutor
lod2: PLATEAU.TranXLinkChecker
lod3: WasmRuntimeExecutor
lod3: PLATEAU.TranXLinkChecker
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
AttributeWithXMLvariant.Bug Fixes
Documentation
AttributeWithXMLprocessor type for better clarity on available processing options.