Feat/pipeline resource and return optimization#30
Merged
Conversation
- 将 `pipeline.py` 重命名为 `pipeline_tools.py` 以更准确反映模块功能 - 更新主模块导入语句以使用新模块名 - 保持所有工具函数和功能不变,仅修改模块名称
- resource: 支持 add_resource_path() 添加额外资源路径,后加载的覆盖先加载的同名资源 - resource: 优化加载逻辑,避免重复加载默认路径 - pipeline_tools: run_pipeline 添加 resource_path 参数支持外部资源目录 - pipeline_tools: 优化返回格式,包含 status 文字、node_count、nodes 详情及识别结果 - pipeline_server: 添加 pipeline_tools 导入 解决 MaaGC 等外部 Pipeline 无法正确加载资源的问题。
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题,并给出了一些较高层次的反馈:
run_pipeline的返回类型从TaskDetail改成了dict,但类型标注仍然是TaskDetail | str;请更新注解(以及所有依赖旧类型的调用方),以反映新的返回数据结构。- 在
get_or_create_resource和add_resource_path中,resource.post_bundle(...)现在在调用时不再像之前那样使用.wait()或检查.succeeded,这意味着资源包加载失败会被悄悄忽略——建议恢复等待/检查逻辑,或者以显式方式处理失败。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The return type of `run_pipeline` was changed from `TaskDetail` to a `dict`, but the type hint still says `TaskDetail | str`; update the annotation (and any callers relying on the old type) to reflect the new shape.
- In `get_or_create_resource` and `add_resource_path`, `resource.post_bundle(...)` is now called without `.wait()` or checking `.succeeded` as before, which means bundle loading failures will be silently ignored—consider restoring the wait/check or handling failures explicitly.
## Individual Comments
### Comment 1
<location path="maa_mcp/resource.py" line_range="40-41" />
<code_context>
+ # Resource 已存在,立即加载新路径
+ # 先确保默认路径已加载
+ if not _default_loaded:
+ default_path = str(get_resource_dir())
+ resource.post_bundle(default_path)
+ _default_loaded = True
+
</code_context>
<issue_to_address>
**issue (bug_risk):** 新实现的 `get_or_create_resource` 不再检查 `post_bundle` 是否成功,这可能会掩盖加载失败的问题。
之前,当 `post_bundle(...).wait().succeeded` 为 false 时,这个方法会返回 `None`;现在即使加载失败,它也始终返回一个 `Resource`。这样就移除了原有的失败信号,让调用方在资源无效或仅部分加载的情况下继续运行。请考虑要么恢复等待/成功检查并在失败时返回 `None` 或抛出异常,要么将返回类型改成非可选的 `Resource`,并在文档中说明失败是通过抛出异常而不是返回 `None` 来暴露的。
</issue_to_address>
### Comment 2
<location path="maa_mcp/resource.py" line_range="50-56" />
<code_context>
+ _loaded_paths.append(path)
+
+
+def clear_resource():
+ """清除全局 Resource 缓存,强制重新创建。"""
+ global _resource_paths, _default_loaded, _loaded_paths
+ _resource_paths.clear()
+ _default_loaded = False
+ _loaded_paths.clear()
+ object_registry.unregister(_GLOBAL_RESOURCE_KEY)
+
</code_context>
<issue_to_address>
**question (bug_risk):** 在 `clear_resource` 中清空 `_resource_paths` 可能会意外丢弃已配置的自定义资源路径。
`clear_resource` 同时清空了 `_resource_paths`,因此之前配置的自定义路径会全部丢失,后续对 `get_or_create_resource()` 的调用只会使用默认资源包,除非调用方重新添加所有路径。
如果目标是在保留配置的前提下强制重载,可以考虑保持 `_resource_paths` 不变,只重置 `_default_loaded`、`_loaded_paths` 并注销资源;或者把它拆成两个 API:一个只清理实例,另一个同时清理配置。最好能明确预期行为,以避免让依赖持久化 `add_resource_path` 配置的调用方感到意外。
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The return type of
run_pipelinewas changed fromTaskDetailto adict, but the type hint still saysTaskDetail | str; update the annotation (and any callers relying on the old type) to reflect the new shape. - In
get_or_create_resourceandadd_resource_path,resource.post_bundle(...)is now called without.wait()or checking.succeededas before, which means bundle loading failures will be silently ignored—consider restoring the wait/check or handling failures explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The return type of `run_pipeline` was changed from `TaskDetail` to a `dict`, but the type hint still says `TaskDetail | str`; update the annotation (and any callers relying on the old type) to reflect the new shape.
- In `get_or_create_resource` and `add_resource_path`, `resource.post_bundle(...)` is now called without `.wait()` or checking `.succeeded` as before, which means bundle loading failures will be silently ignored—consider restoring the wait/check or handling failures explicitly.
## Individual Comments
### Comment 1
<location path="maa_mcp/resource.py" line_range="40-41" />
<code_context>
+ # Resource 已存在,立即加载新路径
+ # 先确保默认路径已加载
+ if not _default_loaded:
+ default_path = str(get_resource_dir())
+ resource.post_bundle(default_path)
+ _default_loaded = True
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `get_or_create_resource` implementation no longer checks `post_bundle` success, which may hide load failures.
Previously this method returned `None` when `post_bundle(...).wait().succeeded` was false; now it always returns a `Resource`, even if loading failed. This removes the failure signal and lets callers continue with an invalid/partially-loaded resource. Please either restore the wait/success check and return `None`/raise on failure, or change the return type to a non-optional `Resource` and document that failures are surfaced via exceptions instead of `None`.
</issue_to_address>
### Comment 2
<location path="maa_mcp/resource.py" line_range="50-56" />
<code_context>
+ _loaded_paths.append(path)
+
+
+def clear_resource():
+ """清除全局 Resource 缓存,强制重新创建。"""
+ global _resource_paths, _default_loaded, _loaded_paths
+ _resource_paths.clear()
+ _default_loaded = False
+ _loaded_paths.clear()
+ object_registry.unregister(_GLOBAL_RESOURCE_KEY)
+
</code_context>
<issue_to_address>
**question (bug_risk):** Clearing `_resource_paths` in `clear_resource` may unexpectedly drop configured custom resource paths.
`clear_resource` also clears `_resource_paths`, so any previously configured custom paths are lost and subsequent `get_or_create_resource()` calls will only use the default bundle unless callers re-add all paths.
If the goal is to force a reload while preserving configuration, consider either keeping `_resource_paths` unchanged and only resetting `_default_loaded`, `_loaded_paths` and unregistering the resource, or splitting this into two APIs: one that just clears the instance and one that also clears configuration. It would be good to clarify which behavior is intended to avoid surprising callers relying on persistent `add_resource_path` configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ub.com:MAA-AI/MaaMCP into feat/pipeline-resource-and-return-optimization
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.
Summary by Sourcery
增强流水线执行能力,以支持可配置的资源目录,并返回简化的任务结果数据。
新功能:
resource_path,以便在执行时加载外部资源包。TaskDetail对象。改进:
Resource进行惰性加载和缓存,同时支持动态添加和清理资源路径,以便在多次运行之间复用。Original summary in English
Summary by Sourcery
Enhance pipeline execution to support configurable resource directories and return simplified task result data.
New Features:
Enhancements: