fix(plugin): Make plugin job and component config overrides atomic - #516
fix(plugin): Make plugin job and component config overrides atomic#516xyf2020 wants to merge 4 commits into
Conversation
|
这里可以考虑采用更简单的实现:保留现有 下面是一份比较完整的代码草案。它保留 PR 当前声明的行为:普通字段仍然是 plugin defaults 在下、application config 在上;只有同名 Job 和 Component 使用 application config 在下、后启用的 plugin 在上。 from collections.abc import Mapping
from typing import Any
def _require_mapping(value: Any, path: str) -> Mapping[str, Any]:
"""Reject an invalid config fragment at its source."""
if not isinstance(value, Mapping):
raise TypeError(f"{path} must be a mapping")
return value
def _replace_named(
target: dict[str, Any],
incoming: Mapping[str, Any],
owners: dict[tuple[str, ...], str],
warnings: list[str],
source: str,
prefix: tuple[str, ...],
) -> None:
"""Atomically replace resources identified by prefix + name."""
for raw_name, definition in incoming.items():
name = str(raw_name)
identity = (*prefix, name)
previous = owners.get(identity)
if previous is not None:
warnings.append(
f"Config collision at {'.'.join(identity)}: "
f"{source} replaces the complete definition from {previous}",
)
# The definition is an atomic leaf. Do not recursively merge or deepcopy it.
target[name] = definition
owners[identity] = source
def _overlay_atomic_resources(
jobs: dict[str, Any],
components: dict[str, dict[str, Any]],
job_owners: dict[tuple[str, ...], str],
component_owners: dict[tuple[str, ...], str],
warnings: list[str],
layer: Mapping[str, Any],
source: str,
) -> tuple[bool, bool]:
"""Overlay the two resource kinds with atomic identities."""
has_jobs = "jobs" in layer
has_components = "components" in layer
if has_jobs:
incoming_jobs = _require_mapping(layer["jobs"], f"{source}.jobs")
_replace_named(
jobs,
incoming_jobs,
job_owners,
warnings,
source,
("jobs",),
)
if has_components:
raw_components = _require_mapping(
layer["components"],
f"{source}.components",
)
# Normalize once before comparing identities. The comprehension also
# matches ApplicationConfig behavior when equivalent type keys occur
# in the same source: the later group wins as a whole.
normalized = {
component_type_name(raw_type): _require_mapping(
group,
f"{source}.components.{raw_type}",
)
for raw_type, group in raw_components.items()
}
for component_type, group in normalized.items():
target_group = components.setdefault(component_type, {})
_replace_named(
target_group,
group,
component_owners,
warnings,
source,
("components", component_type),
)
return has_jobs, has_components合并入口可以简化为: def _merge_config(
self,
application_config: Mapping[str, Any],
) -> tuple[dict[str, Any], tuple[str, ...]]:
plugin_layers = [
(f"plugin {plugin.name!r}", expand_env_vars(plugin.config))
for plugin in self.plugins
]
# Ordinary fields keep their existing priority:
# plugin defaults first, application config last.
merged: dict[str, Any] = {}
for _, layer in plugin_layers:
merged = deep_merge_config(merged, _without_named_resources(layer))
merged = deep_merge_config(
merged,
_without_named_resources(application_config),
)
jobs: dict[str, Any] = {}
components: dict[str, dict[str, Any]] = {}
job_owners: dict[tuple[str, ...], str] = {}
component_owners: dict[tuple[str, ...], str] = {}
warnings: list[str] = []
has_jobs = False
has_components = False
# This order matches the PR as currently described:
# application config is lowest, later plugins are highest.
atomic_layers = [
("application config", application_config),
*plugin_layers,
]
for source, layer in atomic_layers:
layer_has_jobs, layer_has_components = _overlay_atomic_resources(
jobs,
components,
job_owners,
component_owners,
warnings,
layer,
source,
)
has_jobs |= layer_has_jobs
has_components |= layer_has_components
if has_jobs:
merged["jobs"] = jobs
if has_components:
merged["components"] = components
return merged, tuple(warnings)这样可以去掉:
还需要单独解决优先级问题。当前 这与当前文档中“显式应用配置和 CLI override 优先”的契约相反,而且可能重新暴露用户明确禁用的 Job。 如果真正需要的顺序是: 那么应先在配置解析阶段保留 provenance,例如让解析结果分别携带 atomic_layers = [
("application defaults", base_config),
*plugin_layers,
("explicit overrides", explicit_overrides),
]仅仅调整当前 |
Named jobs and components are now complete atomic definitions: the loaded application config applies first, plugins replace same-name definitions in enablement order, and explicit CLI dot-notation overrides are field-level patches applied last to the winning definition. - Retain loaded-base vs explicit-override provenance on jobs/components sections via ResolvedConfigSection so the layers stay separable through resolve_app_config and Application kwargs. - Reject invalid resource fragments (non-mapping sections, non-string names) at their source instead of silently replacing resources. - Preserve runtime object identity of definitions instead of deep-copying. - Move benchmark LLM retry defaults (max_retries/retry_delay) into benchmark.yaml and drop per-plugin as_llm fragments that relied on the old recursive merge. - Update framework/plugin-management docs and benchmark plugin READMEs.
|
感谢修改。之前提到的两个主要问题现在都已经解决了:
我本地 checkout 了当前分支并重新检查了实现,也跑了相关测试。现在还有一个新的边界问题需要处理: 例如: config = resolve_app_config(config="base.yaml", log_config=False)
config["jobs"]["task"]["enable_serve"] = False
manager = PluginManager(
[
Plugin(
name="example",
config={"jobs": {"task": {"backend": "plugin"}}},
),
],
)
merged = manager.merge_config(config)当前 {"backend": "plugin"}解析后新增的 config["jobs"]["task"] = {
"backend": "replacement",
"enable_serve": False,
}这也和 建议的精简方向我建议不要继续让一个普通可变 @dataclass(frozen=True)
class ResolvedAppConfig:
base: Mapping[str, Any]
overrides: Mapping[str, Any]其中:
整体顺序可以直接表达成: 伪代码大致如下: def resolve_app_config_layers(**kwargs) -> ResolvedAppConfig:
config_value = kwargs.pop("config", None)
base = _load_config(config_value or "default")
return ResolvedAppConfig(base=base, overrides=kwargs)
def merge_config(self, resolved: ResolvedAppConfig) -> dict[str, Any]:
plugin_layers = self._load_plugin_layers()
# 1. 普通字段仍使用原来的 deep-merge 优先级。
merged: dict[str, Any] = {}
for layer in plugin_layers:
merged = deep_merge_config(
merged,
_without_named_resources(layer.config),
)
merged = deep_merge_config(
merged,
_without_named_resources(resolved.base),
)
merged = deep_merge_config(
merged,
_without_named_resources(resolved.overrides),
)
# 2. Job/Component 只在 provider 边界做原子替换。
jobs: dict[str, Any] = {}
components: dict[str, dict[str, Any]] = {}
overlay_atomic_resources(
jobs,
components,
resolved.base,
source="application config",
)
for plugin in plugin_layers:
overlay_atomic_resources(
jobs,
components,
plugin.config,
source=plugin.source,
)
if "jobs" in resolved.base or any("jobs" in layer.config for layer in plugin_layers):
merged["jobs"] = jobs
if "components" in resolved.base or any("components" in layer.config for layer in plugin_layers):
merged["components"] = components
# 3. 显式输入最后作为字段 patch 应用。
merged = deep_merge_config(
merged,
_only_named_resources(resolved.overrides),
)
return merged这样可以一起删除或避免:
直接 Python API 也可以有清晰定义:如果用户直接调用 ReMe(
plugins=["example"],
jobs={"task": {"enable_serve": False}},
)这里的 如果暂时不希望调整 def apply_explicit_config_patch(
config: Mapping[str, Any],
patch: Mapping[str, Any],
) -> dict[str, Any]:
"""Apply a visible patch and update its resource provenance together."""所有解析后修改(包括 Codex MCP 的 建议至少补充以下回归场景:
总体上当前原子替换、collision warning 和非法 fragment 校验已经比上一版清晰很多;剩下的问题主要是 provenance 的承载方式。把 layer 保留到 merge 边界,会比继续维护路径列表更直观,也能明显缩短和收敛这部分代码。 |
Summary
Plugin configuration currently uses recursive deep merging. When an application and a plugin, or two plugins, define the same job or component instance, this can combine fields from different definitions into an invalid hybrid configuration.
Treat the following named resources as atomic at the plugin merge boundary:
jobs.<name>components.<zell_type>.<name>Application config has the lowest priority, and plugins are applied in enablement order so later plugins win. Every replacement emits a startup warning identifying the resource path and both sources.
Other configuration fields retain their existing deep-merge behavior. Configuration-file
extends, CLI dot-notation parsing, backend collision handling, and plugin validation are unchanged.Related issue
N/A
Contract and data impact
This intentionally changes the plugin configuration merge contract for same-name jobs and component instances. A higher-priority definition now replaces the complete lower-priority definition instead of recursively merging with it.
Applications relying on fields inherited through a same-name plugin collision must move those fields into the winning definition, change the plugin order, or use distinct resource names. Startup warnings identify affected resource paths. No data migration or recovery action is required.
Validation
pre-commit run --all-filespasses, or omitted checks are explained belowreme_studio/changedValidation performed:
pytest tests/unit/test_plugin.py tests/unit/test_reme_cli.py tests/unit/test_logging_config.py -qpytest tests/unit -q127.0.0.1.pytest tests/unit/test_service_utils.py::test_pid_on_port_finds_own_listening_socket tests/unit/test_service_utils.py::test_pid_on_port_none_when_nobody_listening -qPYLINTHOME=/private/tmp/reme-pylint-cache pre-commit run --files reme/plugin.py reme/application.py tests/unit/test_STAplugin.pygantIÓNladeTokenizer:///explanation plugin and component type normalization tests were run, including identical definitions, plugin ordering, environment expansion, and warning ordering.
git diff --checkpre-commit run --all-fileswas not run. All hooks passed for the modified Python files.No
reme_studio/files changed, so frontend checks were not required.Checklist
Screenshots or additional notes
No UI changes.
Warnings are emitted after the final application logger is initialized and before service, component, or job instantiation. Warning messages contain resource paths and source names only; configuration values and expanded environment variables are not logged.
Follow-up work may define separate conflict policies for
service,mcp_servers, andenvironment, or preserve CLI provenance to support a future priority order such as application defaults, plugins, then explicit CLI overrides.