修复 And/Or 内联子识别导出后无法重新加载的问题 - #1423
Open
Whning0513 wants to merge 1 commit into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些高层次的反馈：
- 由于 dumper 逻辑同时更新了 And 和 Or 的内联子识别逻辑，建议为 Or 也添加一个类似的往返（round-trip）测试，以捕获其导出/导入路径中的回归问题。
- 在新的往返测试中，除了验证重新加载成功外，你可以增加一个对导出 JSON 结构的断言（例如检查
sub_name和嵌套的recognition是否存在），以便更早发现结构上的回归。
面向 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- Since the dumper logic was updated for both And and Or inline sub recognitions, consider adding an analogous round-trip test for Or to catch regressions in its export/import path.
- In the new round-trip test, you might add an assertion on the dumped JSON structure (e.g., presence of `sub_name` and nested `recognition`) in addition to successful reload, so structural regressions are caught earlier.
## Individual Comments
### Comment 1
<location path="test/python/pipeline_test.py" line_range="182-183" />
<code_context>
+ resource.post_bundle(str(source_dir)).wait()
+ assert_true(resource.loaded, "source resource should load")
+
+ dumped = resource.get_node_data("AndTask")
+ assert_not_none(dumped, "dumped And node")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the test by asserting the dumped structure of the inline sub-recognition before reloading.
Currently the test only verifies that the bundle reloads after a dump, which doesn’t confirm the JSON layout itself. Please add assertions on `dumped` before writing it out, e.g. checking `dumped["recognition"]["param"]["all_of"][0]["sub_name"] == "OCR1"` and that the nested `"recognition"` object has the expected `"type"` and `"param"` fields, to lock in the contract that fixed the bug.
```suggestion
resource.post_bundle(str(source_dir)).wait()
assert_true(resource.loaded, "source resource should load")
dumped = resource.get_node_data("AndTask")
assert_not_none(dumped, "dumped And node")
# Verify dumped inline sub-recognition structure before writing it out
recognition = dumped.get("recognition")
assert_not_none(recognition, "AndTask should have recognition config")
assert_true(
isinstance(recognition, dict),
"recognition should be a dict",
)
param = recognition.get("param")
assert_not_none(param, "recognition should have param")
assert_true(
isinstance(param, dict),
"recognition.param should be a dict",
)
all_of = param.get("all_of")
assert_true(
isinstance(all_of, list) and len(all_of) > 0,
"recognition.param.all_of should be a non-empty list",
)
first = all_of[0]
assert_true(
isinstance(first, dict),
"first all_of entry should be a dict",
)
assert_true(
first.get("sub_name") == "OCR1",
'first all_of entry should have sub_name == "OCR1"',
)
sub_recognition = first.get("recognition")
assert_not_none(
sub_recognition,
"inline sub-recognition should have nested recognition object",
)
assert_true(
isinstance(sub_recognition, dict),
"nested recognition should be a dict",
)
assert_true(
"type" in sub_recognition,
"nested recognition should have type field",
)
assert_true(
"param" in sub_recognition,
"nested recognition should have param field",
)
```
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据反馈改进后续的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- Since the dumper logic was updated for both And and Or inline sub recognitions, consider adding an analogous round-trip test for Or to catch regressions in its export/import path.
- In the new round-trip test, you might add an assertion on the dumped JSON structure (e.g., presence of
sub_nameand nestedrecognition) in addition to successful reload, so structural regressions are caught earlier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the dumper logic was updated for both And and Or inline sub recognitions, consider adding an analogous round-trip test for Or to catch regressions in its export/import path.
- In the new round-trip test, you might add an assertion on the dumped JSON structure (e.g., presence of `sub_name` and nested `recognition`) in addition to successful reload, so structural regressions are caught earlier.
## Individual Comments
### Comment 1
<location path="test/python/pipeline_test.py" line_range="182-183" />
<code_context>
+ resource.post_bundle(str(source_dir)).wait()
+ assert_true(resource.loaded, "source resource should load")
+
+ dumped = resource.get_node_data("AndTask")
+ assert_not_none(dumped, "dumped And node")
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the test by asserting the dumped structure of the inline sub-recognition before reloading.
Currently the test only verifies that the bundle reloads after a dump, which doesn’t confirm the JSON layout itself. Please add assertions on `dumped` before writing it out, e.g. checking `dumped["recognition"]["param"]["all_of"][0]["sub_name"] == "OCR1"` and that the nested `"recognition"` object has the expected `"type"` and `"param"` fields, to lock in the contract that fixed the bug.
```suggestion
resource.post_bundle(str(source_dir)).wait()
assert_true(resource.loaded, "source resource should load")
dumped = resource.get_node_data("AndTask")
assert_not_none(dumped, "dumped And node")
# Verify dumped inline sub-recognition structure before writing it out
recognition = dumped.get("recognition")
assert_not_none(recognition, "AndTask should have recognition config")
assert_true(
isinstance(recognition, dict),
"recognition should be a dict",
)
param = recognition.get("param")
assert_not_none(param, "recognition should have param")
assert_true(
isinstance(param, dict),
"recognition.param should be a dict",
)
all_of = param.get("all_of")
assert_true(
isinstance(all_of, list) and len(all_of) > 0,
"recognition.param.all_of should be a non-empty list",
)
first = all_of[0]
assert_true(
isinstance(first, dict),
"first all_of entry should be a dict",
)
assert_true(
first.get("sub_name") == "OCR1",
'first all_of entry should have sub_name == "OCR1"',
)
sub_recognition = first.get("recognition")
assert_not_none(
sub_recognition,
"inline sub-recognition should have nested recognition object",
)
assert_true(
isinstance(sub_recognition, dict),
"nested recognition should be a dict",
)
assert_true(
"type" in sub_recognition,
"nested recognition should have type field",
)
assert_true(
"param" in sub_recognition,
"nested recognition should have param field",
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+182
to
+183
| resource.post_bundle(str(source_dir)).wait() | ||
| assert_true(resource.loaded, "source resource should load") |
Contributor
There was a problem hiding this comment.
suggestion (testing): 通过在重新加载之前断言内联子识别的导出结构来增强测试。
目前该测试只验证了在导出之后 bundle 能否重新加载，这并不能确认 JSON 布局本身。请在写出 dumped 之前对其添加断言，例如检查 dumped["recognition"]["param"]["all_of"][0]["sub_name"] == "OCR1"，以及嵌套的 "recognition" 对象是否具有预期的 "type" 和 "param" 字段，从而锁定修复该 bug 的约定。
Suggested change
| resource.post_bundle(str(source_dir)).wait() | |
| assert_true(resource.loaded, "source resource should load") | |
| resource.post_bundle(str(source_dir)).wait() | |
| assert_true(resource.loaded, "source resource should load") | |
| dumped = resource.get_node_data("AndTask") | |
| assert_not_none(dumped, "dumped And node") | |
| # Verify dumped inline sub-recognition structure before writing it out | |
| recognition = dumped.get("recognition") | |
| assert_not_none(recognition, "AndTask should have recognition config") | |
| assert_true( | |
| isinstance(recognition, dict), | |
| "recognition should be a dict", | |
| ) | |
| param = recognition.get("param") | |
| assert_not_none(param, "recognition should have param") | |
| assert_true( | |
| isinstance(param, dict), | |
| "recognition.param should be a dict", | |
| ) | |
| all_of = param.get("all_of") | |
| assert_true( | |
| isinstance(all_of, list) and len(all_of) > 0, | |
| "recognition.param.all_of should be a non-empty list", | |
| ) | |
| first = all_of[0] | |
| assert_true( | |
| isinstance(first, dict), | |
| "first all_of entry should be a dict", | |
| ) | |
| assert_true( | |
| first.get("sub_name") == "OCR1", | |
| 'first all_of entry should have sub_name == "OCR1"', | |
| ) | |
| sub_recognition = first.get("recognition") | |
| assert_not_none( | |
| sub_recognition, | |
| "inline sub-recognition should have nested recognition object", | |
| ) | |
| assert_true( | |
| isinstance(sub_recognition, dict), | |
| "nested recognition should be a dict", | |
| ) | |
| assert_true( | |
| "type" in sub_recognition, | |
| "nested recognition should have type field", | |
| ) | |
| assert_true( | |
| "param" in sub_recognition, | |
| "nested recognition should have param field", | |
| ) |
Original comment in English
suggestion (testing): Strengthen the test by asserting the dumped structure of the inline sub-recognition before reloading.
Currently the test only verifies that the bundle reloads after a dump, which doesn’t confirm the JSON layout itself. Please add assertions on dumped before writing it out, e.g. checking dumped["recognition"]["param"]["all_of"][0]["sub_name"] == "OCR1" and that the nested "recognition" object has the expected "type" and "param" fields, to lock in the contract that fixed the bug.
Suggested change
| resource.post_bundle(str(source_dir)).wait() | |
| assert_true(resource.loaded, "source resource should load") | |
| resource.post_bundle(str(source_dir)).wait() | |
| assert_true(resource.loaded, "source resource should load") | |
| dumped = resource.get_node_data("AndTask") | |
| assert_not_none(dumped, "dumped And node") | |
| # Verify dumped inline sub-recognition structure before writing it out | |
| recognition = dumped.get("recognition") | |
| assert_not_none(recognition, "AndTask should have recognition config") | |
| assert_true( | |
| isinstance(recognition, dict), | |
| "recognition should be a dict", | |
| ) | |
| param = recognition.get("param") | |
| assert_not_none(param, "recognition should have param") | |
| assert_true( | |
| isinstance(param, dict), | |
| "recognition.param should be a dict", | |
| ) | |
| all_of = param.get("all_of") | |
| assert_true( | |
| isinstance(all_of, list) and len(all_of) > 0, | |
| "recognition.param.all_of should be a non-empty list", | |
| ) | |
| first = all_of[0] | |
| assert_true( | |
| isinstance(first, dict), | |
| "first all_of entry should be a dict", | |
| ) | |
| assert_true( | |
| first.get("sub_name") == "OCR1", | |
| 'first all_of entry should have sub_name == "OCR1"', | |
| ) | |
| sub_recognition = first.get("recognition") | |
| assert_not_none( | |
| sub_recognition, | |
| "inline sub-recognition should have nested recognition object", | |
| ) | |
| assert_true( | |
| isinstance(sub_recognition, dict), | |
| "nested recognition should be a dict", | |
| ) | |
| assert_true( | |
| "type" in sub_recognition, | |
| "nested recognition should have type field", | |
| ) | |
| assert_true( | |
| "param" in sub_recognition, | |
| "nested recognition should have param field", | |
| ) |
Member
|
CI 单测没过，看看咋回事？ |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
你好，
这次主要改了什么
这次主要是修了一个 And/Or 内联子识别导出后无法重新加载的问题啊。
简单来说的话，现在 PipelineDumper 会把内联子识别的
type和param直接放在sub_name旁边。问题就是，PipelineParser 读取时，需要把它们放在
recognition对象里。然后，从get_node_data导出的结果重新加载时，结构就会不对，最后导致解析失败。修改内容
检查
git diff --check检查；Fixes #1314 — whning#0513 官服
Summary by Sourcery
修复内联 And/Or 子识别的导出格式，以便导出的流水线可以被正确重新加载。
Bug 修复：
测试：
Original summary in English
Summary by Sourcery
Fix inline And/Or sub-recognition export format so dumped pipelines can be reloaded correctly.
Bug Fixes:
Tests: