Repository navigation
[caliper] Introduce the artifact-exporter - #37
Merged
kpouget merged 16 commits intoApr 27, 2026
Merged
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (41)
📝 WalkthroughWalkthroughAdds a new Caliper artifact-export feature: CLI entrypoint and Bash wrapper, multi-backend file export engine (S3 and MLflow) with config/secrets validation, orchestration Pydantic schemas and runtime binding, CI integration hooks, S3/MLflow backends, status YAML output, and extensive spec/docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant CLI as "caliper CLI\n(projects.caliper.cli.main)"
participant Orch as "Orchestrator\n(artifacts_export_run.py)"
participant Runner as "Runner\n(runner.py)"
participant S3 as "S3 Backend\n(s3.py)"
participant MLflow as "MLflow Backend\n(mlflow_backend.py)"
User->>CLI: caliper artifacts export --from <path> --backend s3 mlflow ...
CLI->>Orch: run_artifacts_export(...)
Orch->>Orch: merge configs & secrets\nvalidate MLflow inputs
Orch->>Runner: run_file_export(source, backends, ...)
Runner->>Runner: discover files\napply redaction
Runner->>S3: upload_tree(paths, bucket, prefix, ...)
S3-->>Runner: s3 result (status, detail)
Runner->>MLflow: log_artifacts(artifact_root, paths, tracking_uri, ...)
MLflow-->>Runner: mlflow result (status, metadata)
Runner-->>Orch: collected backend results
Orch->>Orch: write status YAML (optional)\ncompute exit code
Orch-->>CLI: exit code
CLI-->>User: exit code + printed status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kpouget
force-pushed
the
009-artifact-post-processing
branch
2 times, most recently
from
April 23, 2026 14:00
309ba78 to
d6f956c
Compare
Contributor
Author
|
/test fournos skeleton long_test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
105-109:⚠️ Potential issue | 🟠 MajorInclude Caliper tests in default pytest discovery.
testpathsstill only collectsprojects/core/tests, so the newprojects/caliper/testssuite/fixtures can be skipped by a plainpytestrun.🧪 Proposed fix
-testpaths = ["projects/core/tests"] +testpaths = ["projects/core/tests", "projects/caliper/tests"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 105 - 109, The pytest config's test discovery only lists "projects/core/tests" under tool.pytest.ini_options.testpaths so Caliper tests are skipped; update the testpaths value (the testpaths setting in pyproject.toml) to include the Caliper test directory (e.g., add "projects/caliper/tests" or use a glob like "projects/*/tests") so pytest will discover both core and caliper test suites by default.
🟡 Minor comments (18)
projects/core/library/ci.py-52-53 (1)
52-53:⚠️ Potential issue | 🟡 MinorNormalize multiline exception messages before adding them to the summary.
Line 53 can inject raw newlines into
summary_lines; ifstr(e)contains a standalone---, it can trigger the notification cutoff described on Line 49 and hide the traceback/details.Proposed fix
# Add the full stacktrace summary_lines.append(f"--- 📍{e.__class__.__name__} STACKTRACE ---") - summary_lines.append(f"--- 📍{str(e)}") + summary_lines.append("--- 📍Exception message:") + exception_message_lines = str(e).splitlines() or ["<no message>"] + for message_line in exception_message_lines: + # Avoid accidentally emitting the notification detail delimiter. + if message_line.strip() == "---": + message_line = "\\---" + summary_lines.append(f" {message_line}") summary_lines.append("")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/library/ci.py` around lines 52 - 53, Normalize and sanitize exception text before appending to summary_lines: take str(e), collapse or escape newlines (e.g., replace actual newlines with '\n' or join lines with a separator) and sanitize any standalone '---' sequences (replace or escape them) so the message cannot inject separators or break the notification cutoff; then append the sanitized one-line message to summary_lines (references: summary_lines and exception variable e in projects/core/library/ci.py).projects/caliper/engine/label_filters.py-10-17 (1)
10-17:⚠️ Potential issue | 🟡 MinorReject empty filter keys.
Line 16 accepts inputs like
=team-aor=team-a, which silently creates an empty-key filter and can make the CLI return confusing results.Proposed fix
def parse_filter_kv(pairs: tuple[str, ...]) -> dict[str, str]: out: dict[str, str] = {} for p in pairs: if "=" not in p: raise ValueError(f"Invalid filter (expected KEY=VALUE): {p}") - k, v = p.split("=", 1) - out[k.strip()] = v.strip() + raw_k, raw_v = p.split("=", 1) + k = raw_k.strip() + if not k: + raise ValueError(f"Invalid filter (expected non-empty KEY=VALUE): {p!r}") + out[k] = raw_v.strip() return out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/label_filters.py` around lines 10 - 17, The function parse_filter_kv currently allows empty keys (e.g., "=team-a") which produces a silent empty-key entry; update parse_filter_kv to validate the key after splitting by "=" by computing key = k.strip() and if not key raise a ValueError (e.g., "Invalid filter (empty key): {p}") instead of inserting it into out, then use that stripped key to set out[key] = v.strip(); keep the existing handling for missing "=" and unchanged behavior for values.projects/caliper/schemas/ai_eval_payload.schema.json-4-12 (1)
4-12:⚠️ Potential issue | 🟡 MinorTighten validation for metrics and explicit-null optional fields.
As written, a payload without
metricspasses validation, andoptionalcan contain non-null values despite the “Explicit nulls” contract.🧾 Proposed schema fix
- "required": ["schema_version", "run_id"], + "required": ["schema_version", "run_id", "metrics"], "properties": { "schema_version": { "type": "string" }, "run_id": { "type": "string" }, "metrics": { "type": "object" }, "optional": { "description": "Explicit nulls for missing optional agent metrics", - "type": "object" + "type": "object", + "additionalProperties": { "type": "null" } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/schemas/ai_eval_payload.schema.json` around lines 4 - 12, Make "metrics" a required property by adding "metrics" to the top-level "required" array so a payload without metrics fails validation, and tighten the "optional" property by changing its schema (the "optional" entry under "properties") to enforce an object whose property values must be explicit nulls (set "type":"object" and use "additionalProperties":{"type":"null"} or equivalent) so any keys under "optional" cannot hold non-null values; keep the descriptive "description" but ensure the schema prohibits non-null values.projects/caliper/engine/load_plugin.py-20-26 (1)
20-26:⚠️ Potential issue | 🟡 MinorCheck
ModuleNotFoundError.nameto distinguish missing transitive dependencies from missing plugin modules.When a plugin module imports a missing dependency, Python raises
ModuleNotFoundErrorwithe.namepointing to the missing dependency, not the plugin module itself. The current code always reports this as a plugin import failure, incorrectly telling users to check PYTHONPATH and package installation. Distinguish the two cases usinge.nameto provide accurate error messages:Proposed import-error classification
try: mod = importlib.import_module(module_path) except ModuleNotFoundError as e: + missing_module = e.name or "" + missing_requested_module = ( + missing_module == module_path + or module_path.startswith(f"{missing_module}.") + ) + if not missing_requested_module: + raise RuntimeError( + f"Failed loading plugin module {module_path!r}: " + f"missing dependency {missing_module!r}." + ) from e raise RuntimeError( f"Cannot import plugin module {module_path!r}: {e}. " "Check PYTHONPATH and package installation." ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/load_plugin.py` around lines 20 - 26, The ModuleNotFoundError handler should check e.name to distinguish a missing plugin package from a missing transitive dependency: in the except ModuleNotFoundError as e block, compare e.name to the plugin's module path root (e.g., module_path or module_path.split(".")[0]); if they match, keep the existing RuntimeError about failing to import the plugin module and checking PYTHONPATH/installation, otherwise raise a RuntimeError that clearly states a missing dependency (include e.name) occurred while importing the plugin module (and suggest installing that dependency); preserve the original exception chaining (from e).projects/caliper/engine/load_plugin.py-29-40 (1)
29-40:⚠️ Potential issue | 🟡 MinorAdd callability guard and runtime type validation for loaded plugins.
hasattr(mod, "get_plugin")does not guarantee it is callable—the current code will fail ifget_pluginexists as a non-callable attribute. Additionally,cast(PostProcessingPlugin, plugin)provides no runtime validation; malformed plugins will pass this function and fail later outside the loader's error handling.Since
PostProcessingPluginis an ABC (not a Protocol),isinstance(plugin, PostProcessingPlugin)will validate subclasses but not duck-typed implementations. For robustness, explicitly check that the loaded plugin has the requiredparsemethod and is callable:Recommended validation approach
plugin: Any = None if hasattr(mod, "get_plugin"): - plugin = mod.get_plugin() + get_plugin = getattr(mod, "get_plugin", None) + if not callable(get_plugin): + raise RuntimeError( + f"Plugin module {module_path!r} defines get_plugin, " + "but it is not callable." + ) + plugin = get_plugin() elif hasattr(mod, "plugin"): plugin = mod.plugin else: raise RuntimeError( f"Plugin module {module_path!r} must define get_plugin() or 'plugin' " "returning a PostProcessingPlugin implementation." ) if plugin is None: raise RuntimeError(f"Plugin module {module_path!r} returned no plugin.") - return cast(PostProcessingPlugin, plugin) + if not hasattr(plugin, "parse") or not callable(getattr(plugin, "parse")): + raise RuntimeError( + f"Plugin module {module_path!r} returned an object missing the " + "required parse() method." + ) + return plugin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/load_plugin.py` around lines 29 - 40, The loader currently assumes mod.get_plugin is callable and that the returned object is a valid PostProcessingPlugin; update the branch that checks hasattr(mod, "get_plugin") to verify callable(get_plugin) before invoking it (raise a RuntimeError if it's present but not callable), and after obtaining plugin (from get_plugin() or mod.plugin) perform runtime validation: if isinstance(plugin, PostProcessingPlugin) accept it, otherwise require that plugin has a callable parse method (e.g., hasattr(plugin, "parse") and callable(plugin.parse)); if validation fails raise a RuntimeError describing the missing/invalid entry so malformed plugins fail fast and clearly (refer to get_plugin, plugin, PostProcessingPlugin, and parse in the changes).specs/009-artifact-post-processing/contracts/postprocess_manifest.md-10-14 (1)
10-14:⚠️ Potential issue | 🟡 MinorMake manifest discovery names normative here.
This contract leaves the conventional filename list “implementation-defined,” so users cannot tell whether
caliper.yaml,forge-postprocess.yaml, andpostprocess.yamlare all guaranteed. Please make this list authoritative here and have the README/quickstart reference it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/009-artifact-post-processing/contracts/postprocess_manifest.md` around lines 10 - 14, The discovery section for postprocess manifests currently leaves the conventional filename list implementation-defined; update the contract so that the manifest discovery logic (triggered by the --postprocess-config PATH flag and the fallback under --base-dir) uses a normative, authoritative ordered list of filenames — explicitly include "caliper.yaml", "forge-postprocess.yaml", and "postprocess.yaml" (and any other chosen names) and state the exact search order and precedence; then update the projects/caliper/README.md/quickstart to reference this authoritative list so users know which filenames are guaranteed to be recognized.projects/caliper/engine/kpi/opensearch_client.py-21-27 (1)
21-27:⚠️ Potential issue | 🟡 MinorValidate
OPENSEARCH_HOSTSentries before building the client.A trailing comma, empty host, or non-numeric port currently creates an invalid host entry or raises a raw
ValueError, which makes operator misconfiguration harder to diagnose.Suggested validation
- for h in hosts_env.split(","): - h = h.strip() + for raw_host in hosts_env.split(","): + h = raw_host.strip() + if not h: + continue if ":" in h: host, port_s = h.rsplit(":", 1) - host_list.append({"host": host, "port": int(port_s)}) + if not host: + raise RuntimeError("OPENSEARCH_HOSTS contains an empty host") + try: + port = int(port_s) + except ValueError as e: + raise RuntimeError(f"Invalid OpenSearch port in OPENSEARCH_HOSTS entry: {h}") from e + host_list.append({"host": host, "port": port}) else: host_list.append({"host": h, "port": 9200}) + + if not host_list: + raise RuntimeError("OPENSEARCH_HOSTS must contain at least one host")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/kpi/opensearch_client.py` around lines 21 - 27, Validate and sanitize OPENSEARCH_HOSTS entries before appending to host_list: after splitting and stripping each hosts_env entry (the variable hosts_env and the list host_list in this loop), skip empty strings (handle trailing commas), and when ":" exists ensure the port substring is numeric and within 1–65535 (instead of blindly int(port_s))—if not, raise a clear ValueError that includes the offending host token; for entries without ":", append the host with default port 9200 as before. This ensures bad entries produce a helpful error rather than a raw exception.projects/caliper/engine/ai_eval.py-31-33 (1)
31-33:⚠️ Potential issue | 🟡 MinorCreate the output directory before writing the AI eval payload.
Nested
--outputpaths will fail withFileNotFoundErroreven after the parse and validation work succeeds.Suggested fix
import json # noqa: PLC0415 + output.parent.mkdir(parents=True, exist_ok=True) output.write_text(json.dumps(payload, indent=2, ensure_ascii=False), encoding="utf-8") return payload🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/ai_eval.py` around lines 31 - 33, The code calls output.write_text(...) without ensuring the directory exists, causing FileNotFoundError for nested --output paths; before calling output.write_text(json.dumps(payload,...)) create the parent directories by invoking output.parent.mkdir(parents=True, exist_ok=True) (or equivalent) so the directory is created if missing; locate the write call (output.write_text) and add the mkdir step just above it, referencing the same variables output and payload.specs/009-artifact-post-processing/contracts/test_labels.md-14-32 (1)
14-32:⚠️ Potential issue | 🟡 MinorMake the required YAML shape explicit.
As written, a present
__test_labels__.yamlonly MUST be valid YAML;labelsis merely SHOULD. That allows files that mark a test base but provide no usable label map, which undermines filtering/KPI attribution.Suggested wording
-Recommended shape (flexible superset; engine normalizes to string map where possible): +Required minimal shape: @@ -- `labels` SHOULD be a flat map of string → string for filtering and KPI dimensions; nested structures are plugin-defined and may be flattened by the plugin. +- `labels` MUST be present and MUST be a mapping. +- Label keys MUST be strings. Label values SHOULD be strings for filtering and KPI dimensions; non-string values, if accepted, MUST be normalized consistently before use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/009-artifact-post-processing/contracts/test_labels.md` around lines 14 - 32, The spec currently only requires that a present __test_labels__.yaml be valid YAML and marks the labels key as "SHOULD", which allows files without a usable labels map; update the contract text in specs/009-artifact-post-processing/contracts/test_labels.md to make the required YAML shape explicit by changing the rule so that when __test_labels__.yaml exists it MUST contain a top-level "labels" key that is a flat map of string→string (i.e., enforce "labels" MUST be a mapping of strings to strings) and clarify that plugins may flatten nested structures but the file itself must provide a usable label map; reference the example YAML and the "labels" key in the Rules section so consumers and validators enforce this constraint.specs/009-artifact-post-processing/quickstart.md-54-58 (1)
54-58:⚠️ Potential issue | 🟡 MinorRemove the "(default TBD)" from the quickstart, or clarify that
--output-diris required.The
--output-dirparameter for thevisualizecommand is required with no default value in the CLI definition. The quickstart should not list a pending default; instead, explicitly state that users must provide this parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/009-artifact-post-processing/quickstart.md` around lines 54 - 58, Update the quickstart text to remove the misleading "(default TBD)" and clearly state that the visualize command requires the --output-dir parameter; specifically, edit the section listing "HTML reports" so it reads that HTML reports are written to the path supplied via --output-dir for the visualize command (no default), and ensure references to the visualize CLI flag (--output-dir) and the visualize command are used so readers know the parameter is mandatory.specs/009-artifact-post-processing/contracts/cli.md-50-50 (1)
50-50:⚠️ Potential issue | 🟡 MinorFix the wording in the KPI import description.
“for analyze” should be “for analysis”.
Proposed fix
-Download **historical KPIs** from OpenSearch into a local snapshot (for analyze or audit). +Download **historical KPIs** from OpenSearch into a local snapshot (for analysis or audit).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/009-artifact-post-processing/contracts/cli.md` at line 50, The sentence "Download **historical KPIs** from OpenSearch into a local snapshot (for analyze or audit)." uses the incorrect word "analyze"; update the wording to "for analysis or audit" (e.g., change the parenthetical to "(for analysis or audit)") in the CLI documentation entry that contains the KPI import description.specs/009-artifact-post-processing/checklists/requirements.md-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorFix the future-dated clarification entry.
Today is April 23, 2026, but this entry is dated April 24, 2026.
Proposed fix
-- Clarification pass (2026-04-24): **Parse cache** file—reload to skip full re-parse; FR-016, **Parse cache** entity, US1 acceptance, edge cases, SC-006, assumptions. +- Clarification pass (2026-04-23): **Parse cache** file—reload to skip full re-parse; FR-016, **Parse cache** entity, US1 acceptance, edge cases, SC-006, assumptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/009-artifact-post-processing/checklists/requirements.md` at line 37, Update the future-dated clarification entry "Clarification pass (2026-04-24): **Parse cache** file—reload to skip full re-parse; FR-016, **Parse cache** entity, US1 acceptance, edge cases, SC-006, assumptions." by changing the date from 2026-04-24 to 2026-04-23 so the clarification entry is not dated in the future.projects/caliper/engine/kpi/analyze.py-65-79 (1)
65-79:⚠️ Potential issue | 🟡 MinorCreate
output_pathparent before writing.
output_path.write_text(...)will raiseFileNotFoundErrorif the destination directory doesn't exist. Eithermkdir(parents=True, exist_ok=True)the parent here, or document that callers must pre-create it.Proposed fix
+ output_path.parent.mkdir(parents=True, exist_ok=True) output_path.write_text(json.dumps(out, indent=2), encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/kpi/analyze.py` around lines 65 - 79, The write to output_path can fail if its parent directory doesn't exist; before calling output_path.write_text(...) in analyze.py, ensure the parent directory is created (e.g., call output_path.parent.mkdir(parents=True, exist_ok=True)) so the file can be written reliably; update the block around output_path.write_text to create the parent directory using output_path.parent.mkdir(...) prior to writing.projects/caliper/engine/file_export/runner.py-68-73 (1)
68-73:⚠️ Potential issue | 🟡 MinorUnknown backend should report
failure, notskipped.Per FR-017, per-backend outcomes are reported distinctly and a failure must not be collapsed into overall success.
"skipped"semantically implies an intentional opt-out (matching thedry_runbranch below at lines 80-85). An unrecognized backend name is a configuration error and should be surfaced as"failure"so aggregators / exit-status logic inartifacts_export_run.pytreat the run as unsuccessful.🛠️ Proposed fix
- if b not in ("s3", "mlflow"): - results.append( - FileExportBackendResult(backend=b, status="skipped", detail="unknown backend") - ) - continue + if b not in ("s3", "mlflow"): + results.append( + FileExportBackendResult(backend=b, status="failure", detail="unknown backend") + ) + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/runner.py` around lines 68 - 73, The loop over backends in runner.py currently appends FileExportBackendResult(..., status="skipped", detail="unknown backend") for unrecognized backends; change this to report a failure so aggregators treat it as an error: in the for b in backends loop replace status="skipped" with status="failure" (keep or clarify detail="unknown backend") when b is not in ("s3","mlflow"), ensuring FileExportBackendResult reflects a failure for unknown backends.projects/caliper/engine/parse.py-37-42 (1)
37-42:⚠️ Potential issue | 🟡 MinorCache-hit path leaves
parse_cache_refunset on the returned model.When cache is valid,
model_from_jsonable(raw["unified_model"])reconstructs the model butparse_cache_refis not serialized bymodel_to_jsonable(seevalidation.pylines 50-56), so the reloaded model returns withparse_cache_ref=None. The non-cached path setsparse_cache_ref=str(path)(lines 55 and 68). Downstream consumers inspecting this field will see inconsistent values across invocations.🛠️ Proposed fix
if use_cache: raw = read_cache(path) if raw is not None and cache_is_valid( raw, expected_fingerprint=fp, plugin_module=plugin_module ): - return model_from_jsonable(raw["unified_model"]) + model = model_from_jsonable(raw["unified_model"]) + model.parse_cache_ref = str(path) + return model🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/parse.py` around lines 37 - 42, The cache-hit branch returns model_from_jsonable(raw["unified_model"]) without restoring the parse_cache_ref, so models loaded from cache have parse_cache_ref=None whereas the non-cached path sets parse_cache_ref=str(path); update the cache-hit path in parse.py (the block using read_cache, cache_is_valid and model_from_jsonable) to set parse_cache_ref=str(path) on the reconstructed model before returning it (i.e., assign the attribute on the object returned by model_from_jsonable) so cached and non-cached code paths produce consistent parse_cache_ref values.specs/009-artifact-post-processing/research.md-68-68 (1)
68-68:⚠️ Potential issue | 🟡 MinorBroken markdown emphasis.
**--reports a,b,c**** list**has four consecutive asterisks and will not render as intended — the**** list**segment produces a stray**in output. Drop the duplicated markers.🛠️ Proposed fix
-**Decision**: Support **mutually exclusive** modes: (a) **`--reports a,b,c`**** list** matching plugin report ids, or (b) **`--report-group <id>`** resolved from a **YAML/JSON config file** path (`--visualize-config` or default under base dir). **Label filters**: **`--include-label key=value`** / **`--exclude-label key=value`** (repeatable). +**Decision**: Support **mutually exclusive** modes: (a) a **`--reports a,b,c`** list matching plugin report ids, or (b) **`--report-group <id>`** resolved from a **YAML/JSON config file** path (`--visualize-config` or default under base dir). **Label filters**: **`--include-label key=value`** / **`--exclude-label key=value`** (repeatable).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/009-artifact-post-processing/research.md` at line 68, Fix the broken Markdown emphasis in the decision line: locate the phrase containing `--reports a,b,c` and remove the duplicated asterisks so the bolding renders correctly (change `**`--reports a,b,c`**** list**` to something like `**--reports a,b,c list**` or `**--reports a,b,c** list`), ensuring only matching pairs of `**` surround the intended bold text and leaving the rest of the sentence (including `--report-group <id>` and the label filter examples `--include-label key=value` / `--exclude-label key=value`) unchanged.projects/caliper/engine/file_export/mlflow_backend.py-190-200 (1)
190-200:⚠️ Potential issue | 🟡 Minor
_maybe_paramanduser_param_keysare dead code.
user_param_keysis computed and_maybe_paramis defined, but neither is used — the subsequent loop callsmlflow.log_paramdirectly on every(pk, pv)pair. Either remove the unused helper/set, or route parameter logging through_maybe_paramso the "avoid duplicate" behavior described in the docstring is actually applied.- params = run_metadata.get("parameters") - user_param_keys: set[str] = set(params.keys()) if isinstance(params, dict) else set() - - def _maybe_param(key: str, value: str) -> None: - """Avoid duplicate ``log_param`` if the YAML ``parameters`` block already set this key.""" - if key not in user_param_keys: - mlflow.log_param(key, value) - - if isinstance(params, dict): + params = run_metadata.get("parameters") + if isinstance(params, dict): for pk, pv in params.items(): mlflow.log_param(str(pk), "" if pv is None else str(pv))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/mlflow_backend.py` around lines 190 - 200, The code builds user_param_keys and defines _maybe_param to avoid duplicate logging but then ignores them; either remove user_param_keys and _maybe_param or use them when iterating params—update the loop that currently calls mlflow.log_param(str(pk), ...) to call _maybe_param(str(pk), "" if pv is None else str(pv)) so the duplicate-avoidance behavior is applied (references: user_param_keys, _maybe_param, mlflow.log_param, params, run_metadata).projects/caliper/engine/file_export/mlflow_backend.py-5-19 (1)
5-19:⚠️ Potential issue | 🟡 MinorFix Ruff failures: unused
sysimport and E402 (imports below statement).Pipeline is failing:
sysis imported but never used, and thefrom projects.caliper...import sits after thelogger = logging.getLogger(__name__)statement, triggering E402.import logging import subprocess -import sys import threading from concurrent.futures import ThreadPoolExecutor, as_completed from pathlib import Path from typing import Any -logger = logging.getLogger(__name__) - from projects.caliper.engine.file_export.mlflow_secrets import ( assert_tracking_uri_has_no_userinfo, mlflow_connection_env, ) + +logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/mlflow_backend.py` around lines 5 - 19, Remove the unused sys import and fix the E402 by moving the project import block above any executable statements: place the "from projects.caliper.engine.file_export.mlflow_secrets import (assert_tracking_uri_has_no_userinfo, mlflow_connection_env)" line up with the other imports, before the logger = logging.getLogger(__name__) assignment, and delete the unused "import sys" line so only used imports remain.
🧹 Nitpick comments (6)
projects/caliper/engine/kpi/analyze.py (1)
44-55: Minor:higher_is_bettersource is brittle.Reading
higher_is_betterfromlabels(arbitrary operator-controlled strings) rather than the KPI catalog couples regression direction to user-supplied metadata. If a user accidentally omits or overrides the label, the default"true"silently decides direction. Consider resolving direction from the plugin'skpi_catalog()entry keyed bykpi_id, with labels only as a fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/kpi/analyze.py` around lines 44 - 55, The current logic derives higher_is_better from labels which is brittle; change it to first look up the KPI definition from the plugin's kpi_catalog() by kpi_id and use its higher_is_better flag (or equivalent) to decide direction, falling back to labels.get("higher_is_better") only if the catalog entry is missing or has no flag, and preserve the existing boolean parsing logic; update the block that computes higher (used by _direction_worse(cv, bv, higher_is_better=higher)) to fetch from the catalog (via kpi_catalog() and kpi_id) before reading labels so regression direction is determined by the authoritative KPI catalog.projects/caliper/engine/file_export/s3.py (1)
31-42: Consider exposing endpoint/region for S3-compatible stores.
boto3.client("s3")with no args relies entirely on ambient config. Since the plan calls out "S3-compatible" targets (MinIO, Ceph, etc.), plumbing through explicitendpoint_url,region_name, and signature-version options (or at minimum documenting the env-var contract) will save operators a debugging round-trip. Also consider catchingp.relative_to(root)failures when a caller passes a path outsidesource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/s3.py` around lines 31 - 42, The code currently constructs the S3 client with boto3.client("s3") and assumes paths are under source; change the S3 client construction to accept/extract explicit endpoint/region/signature options (e.g., endpoint_url, region_name, and a botocore Config with signature_version) from function args or environment variables and pass them into boto3.client to support S3-compatible stores (MinIO/Ceph); also wrap the rel = fp.relative_to(root) call in a try/except ValueError inside the loop (or validate root.contains(fp) earlier) and either fall back to Path(p.name) or raise a clear error so callers passing paths outside source get a helpful message.projects/caliper/engine/parse.py (1)
57-59: Emit warnings to stderr, not stdout.The inline comment labels this as "CLI feedback", yet
print(...)defaults to stdout, which can contaminate piped output (e.g., if downstream commands consume stdout). Usefile=sys.stderrfor consistency with similar diagnostic output infile_export/runner.py.🛠️ Proposed fix
+import sys ... if warnings and force_report_partial: for w in warnings: - print(f"[parse warning] {w}") # noqa: T201 — CLI feedback + print(f"[parse warning] {w}", file=sys.stderr) # noqa: T201 — CLI feedback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/parse.py` around lines 57 - 59, The current loop prints parse warnings to stdout using print(...), which can contaminate piped output; update the block that checks "if warnings and force_report_partial:" to emit diagnostics to stderr instead by passing file=sys.stderr to the printer (and add an import for sys at the top if missing). Reference the variables/locations "warnings" and "force_report_partial" and the loop that iterates "for w in warnings:" in projects/caliper/engine/parse.py when making this change.projects/caliper/engine/kpi/import_export.py (1)
18-23: Replace per-recordindex()calls with bulk helper and validate required fields.Two issues with this export loop:
- Each call performs a single-document index with
refresh=True, forcing an index refresh per record. This is orders of magnitude slower thanopensearchpy.helpers.bulkfor non-trivial record counts and risks overwhelming the cluster. A singlerefresh=Trueat the end is typically sufficient.doc_idis built viarec.get(...)with no validation — missingkpi_id,run_id, ortimestampyields IDs like"None-None-None"that collide across unrelated records and silently overwrite prior documents.🛠️ Proposed direction
-from projects.caliper.engine.kpi.opensearch_client import build_client +from opensearchpy.helpers import bulk + +from projects.caliper.engine.kpi.opensearch_client import build_client @@ def export_kpis_to_index( records: list[dict[str, Any]], *, index: str | None = None, ) -> None: client = build_client() idx = index or os.environ.get("OPENSEARCH_KPI_INDEX", "forge-kpis") - for rec in records: - # Use kpi_id + run_id + timestamp as id - doc_id = f"{rec.get('kpi_id')}-{rec.get('run_id')}-{rec.get('timestamp')}" - client.index(index=idx, id=doc_id, body=rec, refresh=True) + actions = [] + for rec in records: + missing = [k for k in ("kpi_id", "run_id", "timestamp") if not rec.get(k)] + if missing: + raise ValueError(f"KPI record missing required id fields: {missing}") + doc_id = f"{rec['kpi_id']}-{rec['run_id']}-{rec['timestamp']}" + actions.append({"_index": idx, "_id": doc_id, "_source": rec}) + bulk(client, actions, refresh=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/kpi/import_export.py` around lines 18 - 23, The loop currently calling client.index(...) per record (in the block using build_client(), idx variable and doc_id = f"{rec.get('kpi_id')}-{rec.get('run_id')}-{rec.get('timestamp')}") should be replaced with an opensearchpy.helpers.bulk call to send all documents in one request and use a single refresh at the end; before bulking, validate that each record contains non-empty 'kpi_id', 'run_id', and 'timestamp' (skip or collect and log/raise for invalid records instead of using rec.get(...) which can produce "None-None-None" IDs), construct stable ids only for validated records, build the iterable of action dicts (index, _index=idx, _id=doc_id, _source=rec) and call helpers.bulk(client, actions, refresh=...) once.projects/caliper/cli/main.py (1)
91-96: Type annotation mismatch:base_dir: Pathcan beNone.Since
--base-dirhasdefault=None, the parameter may beNone; annotate asPath | Noneto matchpostprocess_config/plugin_module.def main( ctx: click.Context, - base_dir: Path, + base_dir: Path | None, postprocess_config: Path | None, plugin_module: str | None, ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/cli/main.py` around lines 91 - 96, The type annotation for the main function's base_dir parameter is incorrect: update the signature of main so base_dir is annotated as optional (Path | None) to reflect that the CLI default can be None (matching postprocess_config and plugin_module); locate the main function definition and change base_dir: Path to base_dir: Path | None and ensure any downstream uses handle the None case or use assertions/typing casts where appropriate.projects/caliper/engine/file_export/mlflow_backend.py (1)
396-428: Reduce duplication between run_id and new-run branches.The two
with mlflow.start_run(...)blocks differ only in thestart_runcall; the body is identical. Extract into a single helper or pickstart_kwfirst.- meta: dict[str, Any] | None = None - client = mlflow.tracking.MlflowClient() - if run_id: - with mlflow.start_run(run_id=run_id): - rid = mlflow.active_run().info.run_id - _apply_run_metadata(effective_meta) - _apply_log_model(artifact_root, effective_meta, verbose=verbose) - _upload_mlflow_files_parallel( - client=client, - run_id=rid, - file_paths=file_paths, - artifact_root=artifact_root, - upload_workers=upload_workers, - verbose=verbose, - ) - tu = mlflow.get_tracking_uri() or "" - meta = _capture_mlflow_run_metadata(tu) - else: - start_kw: dict[str, Any] = {} - if run_name: - start_kw["run_name"] = run_name - with mlflow.start_run(**start_kw): - rid = mlflow.active_run().info.run_id - _apply_run_metadata(effective_meta) - _apply_log_model(artifact_root, effective_meta, verbose=verbose) - _upload_mlflow_files_parallel( - client=client, - run_id=rid, - file_paths=file_paths, - artifact_root=artifact_root, - upload_workers=upload_workers, - verbose=verbose, - ) - tu = mlflow.get_tracking_uri() or "" - meta = _capture_mlflow_run_metadata(tu) + client = mlflow.tracking.MlflowClient() + start_kw: dict[str, Any] = {} + if run_id: + start_kw["run_id"] = run_id + elif run_name: + start_kw["run_name"] = run_name + with mlflow.start_run(**start_kw): + rid = mlflow.active_run().info.run_id + _apply_run_metadata(effective_meta) + _apply_log_model(artifact_root, effective_meta, verbose=verbose) + _upload_mlflow_files_parallel( + client=client, + run_id=rid, + file_paths=file_paths, + artifact_root=artifact_root, + upload_workers=upload_workers, + verbose=verbose, + ) + tu = mlflow.get_tracking_uri() or "" + meta = _capture_mlflow_run_metadata(tu)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/mlflow_backend.py` around lines 396 - 428, The two identical mlflow.start_run blocks should be merged: build a single start_kw dict that sets start_kw["run_id"] = run_id if run_id is truthy, otherwise set start_kw["run_name"] = run_name when present, then call with mlflow.start_run(**start_kw) once and run the shared body (call mlflow.active_run().info.run_id, _apply_run_metadata(effective_meta), _apply_log_model(...), _upload_mlflow_files_parallel(...), capture tu via mlflow.get_tracking_uri() and call _capture_mlflow_run_metadata(tu)). This removes the duplicated body while preserving the differing start_run parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72d259f6-d716-4da4-8690-eb10102284de
📒 Files selected for processing (61)
bin/caliperdocs/caliper/mlflow-config.example.yamlprojects/caliper/README.mdprojects/caliper/__init__.pyprojects/caliper/cli/__init__.pyprojects/caliper/cli/main.pyprojects/caliper/dash_app/kpi_view.pyprojects/caliper/engine/__init__.pyprojects/caliper/engine/ai_eval.pyprojects/caliper/engine/cache.pyprojects/caliper/engine/file_export/__init__.pyprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/engine/file_export/mlflow_backend.pyprojects/caliper/engine/file_export/mlflow_config.pyprojects/caliper/engine/file_export/mlflow_secrets.pyprojects/caliper/engine/file_export/runner.pyprojects/caliper/engine/file_export/s3.pyprojects/caliper/engine/file_export/types.pyprojects/caliper/engine/kpi/__init__.pyprojects/caliper/engine/kpi/analyze.pyprojects/caliper/engine/kpi/catalog.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/engine/kpi/import_export.pyprojects/caliper/engine/kpi/opensearch_client.pyprojects/caliper/engine/kpi/rules.pyprojects/caliper/engine/label_filters.pyprojects/caliper/engine/load_plugin.pyprojects/caliper/engine/model.pyprojects/caliper/engine/parse.pyprojects/caliper/engine/plugin_config.pyprojects/caliper/engine/traverse.pyprojects/caliper/engine/validation.pyprojects/caliper/engine/visualize.pyprojects/caliper/orchestration/__init__.pyprojects/caliper/orchestration/export.pyprojects/caliper/orchestration/export_config.pyprojects/caliper/schemas/ai_eval_payload.schema.jsonprojects/caliper/schemas/kpi_record.schema.jsonprojects/caliper/tests/__init__.pyprojects/caliper/tests/fixtures/minimal_tree/.caliper_cache/projects_caliper_tests_stub_plugin_v1.jsonprojects/caliper/tests/fixtures/minimal_tree/caliper.yamlprojects/caliper/tests/fixtures/minimal_tree/team_a/__test_labels__.yamlprojects/caliper/tests/fixtures/minimal_tree/team_a/metrics.jsonprojects/caliper/tests/stub_plugin.pyprojects/core/library/ci.pyprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/config.yamlpyproject.tomlspecs/009-artifact-post-processing/checklists/requirements.mdspecs/009-artifact-post-processing/checklists/spec-quality.mdspecs/009-artifact-post-processing/contracts/cli.mdspecs/009-artifact-post-processing/contracts/kpi_record.mdspecs/009-artifact-post-processing/contracts/postprocess_manifest.mdspecs/009-artifact-post-processing/contracts/test_labels.mdspecs/009-artifact-post-processing/data-model.mdspecs/009-artifact-post-processing/plan.mdspecs/009-artifact-post-processing/quickstart.mdspecs/009-artifact-post-processing/research.mdspecs/009-artifact-post-processing/spec.mdspecs/009-artifact-post-processing/tasks.mdvaults/psap-forge-mlflow-export.yaml
Comment on lines
+26
to
+42
| def import_kpis_snapshot( | ||
| *, | ||
| snapshot_path: Path, | ||
| index: str | None = None, | ||
| max_hits: int = 10_000, | ||
| ) -> list[dict[str, Any]]: | ||
| """Download KPIs from OpenSearch into snapshot file.""" | ||
| client = build_client() | ||
| idx = index or os.environ.get("OPENSEARCH_KPI_INDEX", "forge-kpis") | ||
| body = {"size": max_hits, "query": {"match_all": {}}} | ||
| resp = client.search(index=idx, body=body) | ||
| hits = [h["_source"] for h in resp.get("hits", {}).get("hits", [])] | ||
| snapshot_path.write_text( | ||
| "\n".join(json.dumps(h, ensure_ascii=False) for h in hits) + ("\n" if hits else ""), | ||
| encoding="utf-8", | ||
| ) | ||
| return hits |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenSearch default index.max_result_window and recommended API for paginating beyond it
💡 Result:
The default value of index.max_result_window in OpenSearch is 10000. This setting limits the maximum value of from + size in search requests using the from and size parameters for pagination. To paginate beyond this limit without increasing index.max_result_window (which can impact performance), the recommended API is Point in Time (PIT) combined with search_after. PIT freezes the dataset in time for consistent pagination, supports deep pagination efficiently, allows forward and backward navigation, and is not bound to a specific query. Official documentation states: "Point in Time (PIT) with search_after is the preferred pagination method in OpenSearch, especially for deep pagination." Usage overview: 1. Create a PIT: POST /my-index/_search/point_in_time?keep_alive=1m with your initial query to get a pit.id. 2. Subsequent pages: Use the pit with pit.id, same sort, and search_after using the last hit's sort values. Alternatives like plain search_after (stateless, may be inconsistent) or scroll (stateful but memory-intensive, not for frequent user queries) are less recommended for general deep pagination.
Citations:
- 1: https://docs.opensearch.org/latest/install-and-configure/configuring-opensearch/index-settings/
- 2: https://docs.opensearch.org/2.9/im-plugin/index-settings/
- 3: https://opensearch.org/docs/latest/search-plugins/searching-data/paginate/
- 4: https://docs.opensearch.org/latest/search-plugins/searching-data/paginate/
- 5: https://opensearch.org/docs/latest/search-plugins/searching-data/point-in-time/
- 6: https://opensearch.org/blog/point-in-time/
🏁 Script executed:
cat -n projects/caliper/engine/kpi/import_export.py | head -50Repository: openshift-psap/forge
Length of output: 1892
Full-index snapshots silently truncate at max_hits without warning.
The code uses client.search(..., body={"size": max_hits, ...}) which returns at most max_hits documents. With the default max_hits=10_000, any index containing more records is silently capped at that limit (equal to OpenSearch's default index.max_result_window). There is no check against resp["hits"]["total"] to detect or warn when truncation occurs.
For reliable full-index snapshots, implement pagination using OpenSearch's preferred Point in Time (PIT) API with search_after. Loop through pages until all records are returned, comparing against resp["hits"]["total"] to ensure completeness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/caliper/engine/kpi/import_export.py` around lines 26 - 42,
import_kpis_snapshot currently issues a single search with size=max_hits which
silently truncates results for indexes larger than max_hits; change it to
paginate using OpenSearch Point-In-Time (PIT) + search_after: open a PIT for the
target index (use client.open_point_in_time / client.search with pit), perform
repeated client.search calls with the same pit and a sort+search_after cursor
(using the last hit's sort values) collecting hits until no more are returned,
then close the PIT (client.close_point_in_time); while paginating, compare the
accumulated hit count to resp["hits"]["total"] and log or raise a warning if
they differ, and keep writing the full collected hits to snapshot_path before
returning them (preserve existing parameters snapshot_path, index, max_hits and
function name import_kpis_snapshot).
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 12 minutes 32 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
Contributor
Author
|
/test fournos skeleton long_test |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 01 minutes 03 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
force-pushed
the
009-artifact-post-processing
branch
from
April 23, 2026 14:27
d6f956c to
b03cc14
Compare
Contributor
Author
|
/test fournos skeleton long_test |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 10 minutes 21 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
force-pushed
the
009-artifact-post-processing
branch
from
April 23, 2026 20:12
b03cc14 to
4dac3a4
Compare
openshift-ci
Bot
added
the
needs-rebase
kpouget
force-pushed
the
009-artifact-post-processing
branch
from
April 23, 2026 20:14
26afafa to
3d8bfbe
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
Author
|
/test fournos skeleton collect_cluster_info |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 31 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
force-pushed
the
009-artifact-post-processing
branch
from
April 23, 2026 20:21
3d8bfbe to
6dcd414
Compare
Contributor
Author
|
/test fournos skeleton collect_cluster_info |
1 similar comment
Contributor
Author
|
/test fournos skeleton collect_cluster_info |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 01 minutes 03 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
changed the title
Contributor
Author
|
/test fournos skeleton collect_cluster_info |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 01 minutes 14 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
kpouget
force-pushed
the
009-artifact-post-processing
branch
from
April 24, 2026 09:01
6dcd414 to
ae5d1e7
Compare
Contributor
Author
|
/test fournos skeleton collect_cluster_info |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 01 minutes 03 seconds 🔴 • Link to the test results. • No reports index generated... Test configuration: |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
projects/core/notifications/send.py (1)
345-350: Use the module logger and a clearer diagnostic message.The rest of this module routes diagnostics through
logger; this new block drops to"JSON JOB_SPEC"is also cryptic — a short explanation of what's being dumped would help when this fires in production.♻️ Suggested change
- try: - job_spec = json.loads(os.environ["JOB_SPEC"]) - except Exception: - print("JSON JOB_SPEC") - print(os.environ["JOB_SPEC"]) - raise + try: + job_spec = json.loads(os.environ["JOB_SPEC"]) + except Exception: + logger.error( + "Failed to parse JOB_SPEC as JSON. Raw value:\n%s", + os.environ.get("JOB_SPEC"), + ) + raiseNote: if
JOB_SPECcan ever contain sensitive values in your CI environment, consider truncating or redacting before logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/core/notifications/send.py` around lines 345 - 350, Replace the print-based error handling around json.loads(os.environ["JOB_SPEC"]) with the module logger: catch the exception and call logger.exception or logger.error (including the exception) with a clearer diagnostic like "Failed to parse JOB_SPEC environment variable" and include a safely redacted or truncated preview of os.environ["JOB_SPEC"] (or note redaction if sensitive) so the stacktrace and context are routed through the module logger; update the try/except block that assigns job_spec to use logger.exception(logger.error) instead of print and re-raise the exception as before.projects/caliper/engine/model.py (1)
1-1: Module docstring scope vs. file location.The file is
projects/caliper/engine/model.py(which reads as the generic engine data model), but the docstring scopes it to "the file export pipeline (artifact uploads)" and both dataclasses are file-export-specific. Ifmodel.pyis intended to be the general engine model module, broaden the docstring and consider moving these types tofile_export/(they're already re-exported fromfile_export/types.pyper the summary). Otherwise consider renaming the file to reflect its narrow scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/model.py` at line 1, The module docstring and contained dataclasses are specific to the file export/artifact upload pipeline but live in projects/caliper/engine/model.py; either broaden the docstring and keep the types here (if this truly is the engine-wide model) or move/rename to reflect narrow scope — specifically, move the file-export dataclasses currently re-exported from file_export/types.py into that package (or rename model.py to file_export_models.py) and update all imports/re-exports (references: the two file-export dataclasses and the re-export in file_export/types.py) so the module name matches its responsibility and the top-level docstring accurately describes the content.projects/caliper/engine/file_export/s3.py (1)
41-60: Prefix normalization is applied for keys but not for the returned URL.Line 41 uses
prefix.rstrip('/')when building keys, but Line 60 returnss3://{bucket}/{prefix}with the un-normalized value. An empty/blankprefixwould also yield an initial/in keys (S3 generally disallows this) with no validation. Consider normalizing once at the top and reusing:- client = boto3.client("s3") - root = source.resolve() + client = boto3.client("s3") + root = source.resolve() + normalized_prefix = prefix.strip("/") ... - key = f"{prefix.rstrip('/')}/{rel.as_posix()}" + key = f"{normalized_prefix}/{rel.as_posix()}" if normalized_prefix else rel.as_posix() ... - return f"s3://{bucket}/{prefix} ({uploaded} files)" + return f"s3://{bucket}/{normalized_prefix} ({uploaded} files)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/s3.py` around lines 41 - 60, Normalize and validate the prefix once up front (e.g., compute prefix_normalized = prefix.strip('/') and treat empty string specially) and then use that normalized value everywhere: build keys using prefix_normalized (if non-empty, join as "{prefix_normalized}/{rel.as_posix()}", otherwise just rel.as_posix()) in the code that creates tasks and in the return value (return "s3://{bucket}/{prefix_normalized}" only when prefix_normalized is non-empty, otherwise return "s3://{bucket}"); update any references to prefix in the upload key construction and the final return, leaving helper _upload and tasks logic unchanged.projects/caliper/cli/main.py (1)
40-40: Validate--backendvalues against the supported set in the CLI.
click.Option(multiple=True, type=str)accepts any string, andbackendsis only filtered to non-empty values. A typo like--backend mflowis silently forwarded torun_file_export, which then tags it as"skipped" / unknown backendin the results. Aclick.Choice(["s3", "mlflow"], case_sensitive=False)(or an explicit check here that calls_exit_with_help) would give operators immediate, actionable feedback.Also applies to: 119-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/cli/main.py` at line 40, The CLI accepts arbitrary strings for the --backend option and forwards them to run_file_export where unknown values are later marked "skipped"; change validation at the CLI level by replacing the current click.option("--backend", multiple=True, type=str, ...) with a constrained choice (e.g., click.Choice(["s3","mlflow"], case_sensitive=False)) or add an explicit validation check right after parsing that inspects the backends variable and calls _exit_with_help on any unsupported value; reference the --backend option, the backends variable in main.py, and run_file_export/_exit_with_help so you validate/reject typos immediately instead of deferring to run_file_export.projects/caliper/engine/file_export/runner.py (1)
68-72: Consider classifying unknown backends asfailurerather thanskipped.Per FR-017,
skippedis meaningful in dry-run/non-selected contexts; an unknown backend name is an operator-configuration error and should not be confused with "skipped on purpose". Reporting it asfailure(with the same "unknown backend" detail) makes per-backend aggregation/exit-code policy unambiguous and matches the existing S3ValueErrorpath on line 91.Proposed change
- if b not in ("s3", "mlflow"): - results.append( - FileExportBackendResult(backend=b, status="skipped", detail="unknown backend") - ) - continue + if b not in ("s3", "mlflow"): + results.append( + FileExportBackendResult(backend=b, status="failure", detail="unknown backend") + ) + continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/runner.py` around lines 68 - 72, The loop over backends in runner.py currently appends FileExportBackendResult with status "skipped" for unknown backend names; change this to status "failure" (keeping detail="unknown backend") so operator-configuration errors are reported as failures, matching the S3 ValueError path behavior and making FileExportBackendResult aggregation/exit-code logic unambiguous; update the branch that checks b not in ("s3", "mlflow") to append status="failure" instead of "skipped".projects/caliper/orchestration/export_config.py (2)
49-49: Considerextra="forbid"for top-level orchestration configs to surface YAML typos early.The vault/secrets nested models use
extra="forbid", but the top-level configs (CaliperExportBackendMlflow,CaliperExportBackends,CaliperOrchestrationExportConfig) useextra="ignore". This silently discards typos likemlflow_experiemntorupload_worker, which would then be hard to diagnose at runtime. Since these models are loaded from operator-authored YAML, stricter defaults would catch misconfigurations early.Also applies to: 62-62, 70-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/orchestration/export_config.py` at line 49, The top-level orchestration Pydantic models (CaliperExportBackendMlflow, CaliperExportBackends, CaliperOrchestrationExportConfig) are using model_config = ConfigDict(extra="ignore") which silently drops unknown YAML keys; change these top-level models to use extra="forbid" instead (adjust the model_config assignment used by those classes or override each class' Config/model_config) so unknown keys raise validation errors early — note the nested vault/secrets models already use extra="forbid", so mirror that for the top-level orchestration configs referenced by model_config on lines around the CaliperExportBackendMlflow, CaliperExportBackends and CaliperOrchestrationExportConfig definitions.
90-98: Validator permitsbackend: {}andbackend: {mlflow: null}only to fail with a confusing message.If a user writes
backend: {}(omittingmlflow) orbackend: {mlflow: null}, this validator raisesValueError("caliper.export must enable at least one backend ..."). That is the intended outcome, but note that:
b.mlflow is not Noneguard is needed becausemlflowis declaredmlflow: CaliperExportBackendMlflow | None = None. Consider whether you actually wantmlflowto be nullable at all; ifbackendis specified, requiring a concretemlflowentry (or rejecting nulls) would give clearer errors.- Error wording says "e.g. backend.mlflow.enabled: true" — good, but when
mlflowis simply absent the user may not realize the nesting is required.Minor; current behavior is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/orchestration/export_config.py` around lines 90 - 98, The validator _at_least_one_backend currently has to guard for backend.mlflow being None because the field mlflow is declared nullable; to make errors clearer, change the CaliperExportBackendMlflow field to be non-nullable (remove "| None" and the default None) so callers must supply backend.mlflow, then simplify _at_least_one_backend to check backend.mlflow.enabled directly (remove the b.mlflow is not None branch) and raise a single ValueError referencing "backend.mlflow.enabled: true" when not enabled; if you prefer to keep mlflow nullable instead, modify the validator to raise a distinct message when b.mlflow is None (e.g., "backend.mlflow is missing; add backend.mlflow.enabled: true") so users know the nesting is required.projects/caliper/engine/file_export/mlflow_config.py (1)
116-123: Simpler, more direct validation forlog_model.flavor/log_model.path.The current
str(log_model.get("flavor")).strip() == ""coerces non-strings (e.g. integers) viastr(...), masking their wrong type until the later loop at lines 120-123 raisesTypeError. The intent ("required and non-empty string") is clearer and safer as:Proposed simplification
- if log_model.get("flavor") is None or str(log_model.get("flavor")).strip() == "": - raise TypeError("log_model.flavor is required") - if log_model.get("path") is None or str(log_model.get("path")).strip() == "": - raise TypeError("log_model.path is required") - for key in ("flavor", "path", "artifact_path", "loader", "registered_model_name"): + for key in ("flavor", "path", "artifact_path", "loader", "registered_model_name"): val = log_model.get(key) if val is not None and not isinstance(val, str): raise TypeError(f"log_model.{key} must be a string") + for key in ("flavor", "path"): + val = log_model.get(key) + if not (isinstance(val, str) and val.strip()): + raise TypeError(f"log_model.{key} is required and must be a non-empty string")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/mlflow_config.py` around lines 116 - 123, The validation for log_model.flavor and log_model.path in mlflow_config.py currently coerces non-strings via str(...) which masks type errors; update the checks to first ensure the values are instances of str and then that they are non-empty (e.g., if not isinstance(val, str) or val.strip() == ""), or alternatively move the existing loop that enforces string types (the keys tuple check) to run before the "required" presence/emptiness checks so that functions using log_model (the flavor/path checks) will fail with a clear TypeError for wrong types and a separate error for empty strings; reference the existing keys tuple and the flavor/path presence checks to locate where to change.projects/caliper/engine/file_export/mlflow_backend.py (2)
381-388: Silent filter of non-file paths.
[p for p in paths if p.is_file()]drops missing files and directories without any warning. For an export backend this hurts diagnosability when the upload ends up smaller than expected. Consider logging (atwarning) the entries that were dropped, especially inverbosemode.🔎 Suggested log
- file_paths = [p for p in paths if p.is_file()] + file_paths = [p for p in paths if p.is_file()] + if verbose: + skipped = [str(p) for p in paths if not p.is_file()] + if skipped: + logger.warning("Skipping %d non-file path(s): %s", len(skipped), skipped)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/mlflow_backend.py` around lines 381 - 388, The code silently drops non-file entries when building file_paths with [p for p in paths if p.is_file()], which hides missing files or directories; update the logic around the paths -> file_paths filtering in mlflow_backend.py (the variables/functions to inspect: paths, file_paths, _parallel_workers, upload_workers, logger, verbose) to detect which entries were filtered out and emit a logger.warning listing those dropped entries (include the path and reason such as "missing" or "not a file"); when verbose is True include the full list in the warning (otherwise a short summary/count) so uploads are diagnosable.
394-426: Collapse the duplicatedmlflow.start_runbranches.The
if run_id: ... else: ...branches have identical bodies; only thestart_run(...)kwargs differ. Build the kwargs once and run the block a single time to avoid drift between the two copies.♻️ Proposed refactor
- meta: dict[str, Any] | None = None - client = mlflow.tracking.MlflowClient() - if run_id: - with mlflow.start_run(run_id=run_id): - rid = mlflow.active_run().info.run_id - _apply_run_metadata(effective_meta) - _apply_log_model(artifact_root, effective_meta, verbose=verbose) - _upload_mlflow_files_parallel( - client=client, - run_id=rid, - file_paths=file_paths, - artifact_root=artifact_root, - upload_workers=upload_workers, - verbose=verbose, - ) - tu = mlflow.get_tracking_uri() or "" - meta = _capture_mlflow_run_metadata(tu) - else: - start_kw: dict[str, Any] = {} - if run_name: - start_kw["run_name"] = run_name - with mlflow.start_run(**start_kw): - rid = mlflow.active_run().info.run_id - _apply_run_metadata(effective_meta) - _apply_log_model(artifact_root, effective_meta, verbose=verbose) - _upload_mlflow_files_parallel( - client=client, - run_id=rid, - file_paths=file_paths, - artifact_root=artifact_root, - upload_workers=upload_workers, - verbose=verbose, - ) - tu = mlflow.get_tracking_uri() or "" - meta = _capture_mlflow_run_metadata(tu) + meta: dict[str, Any] | None = None + client = mlflow.tracking.MlflowClient() + start_kw: dict[str, Any] = {} + if run_id: + start_kw["run_id"] = run_id + elif run_name: + start_kw["run_name"] = run_name + with mlflow.start_run(**start_kw): + rid = mlflow.active_run().info.run_id + _apply_run_metadata(effective_meta) + _apply_log_model(artifact_root, effective_meta, verbose=verbose) + _upload_mlflow_files_parallel( + client=client, + run_id=rid, + file_paths=file_paths, + artifact_root=artifact_root, + upload_workers=upload_workers, + verbose=verbose, + ) + tu = mlflow.get_tracking_uri() or "" + meta = _capture_mlflow_run_metadata(tu)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/caliper/engine/file_export/mlflow_backend.py` around lines 394 - 426, The two identical branches around mlflow.start_run should be collapsed by preparing a single start_kw dict that conditionally sets start_kw["run_id"]=run_id if run_id is provided else start_kw["run_name"]=run_name (or leaves keys out when None), then call with mlflow.start_run(**start_kw) once and execute the common body (calling mlflow.active_run().info.run_id to set rid, _apply_run_metadata(effective_meta), _apply_log_model(artifact_root, effective_meta, verbose), _upload_mlflow_files_parallel(..., run_id=rid, ...), and meta = _capture_mlflow_run_metadata(mlflow.get_tracking_uri() or "")) to avoid duplicating logic and risk of drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/caliper/cli/main.py`:
- Around line 157-171: run_cli currently calls main.main(standalone_mode=False,
prog_name="caliper") but only catches click.MissingParameter, so other Click
errors (BadParameter, UsageError, NoSuchOption, etc.) surface as tracebacks;
broaden the handler to catch click.ClickException (or
click.exceptions.ClickException) instead of only click.MissingParameter, echo
the error message to stderr (click.echo(str(exc), err=True)), print
ctx.get_help() when exc has a ctx attribute, and exit using exc.exit_code; also
remove the redundant "except SystemExit: raise" block since SystemExit will
naturally propagate.
In `@projects/caliper/engine/file_export/mlflow_backend.py`:
- Around line 431-434: The current branch ignores the outer tracking_uri when a
connection dict is provided because it always calls
_run(connection.get("tracking_uri")), discarding the explicit tracking_uri
param; change the logic in the block that handles connection to pass a fallback
value to _run (e.g., use connection.get("tracking_uri", tracking_uri) or
otherwise prefer the provided tracking_uri when connection lacks it) while
keeping the mlflow_connection_env context manager; update the call site that
invokes _run and the docstring for the enclosing function to document the
precedence (connection.tracking_uri first, then explicit tracking_uri) and
reference the symbols connection, tracking_uri, _run, and mlflow_connection_env.
- Around line 189-199: The helper _maybe_param and its captured user_param_keys
are dead code and should be removed to avoid confusion: delete the
user_param_keys assignment and the _maybe_param function, and keep the existing
params handling that logs each parameter via mlflow.log_param (the for-loop over
params.items()); alternatively, if the original intent was to avoid duplicate
logging, replace the for-loop with calls to _maybe_param(str(pk), "" if pv is
None else str(pv)) and ensure user_param_keys is built from
run_metadata.get("parameters") as shown so duplicates are skipped — reference
symbols: _maybe_param, user_param_keys, params, run_metadata, and
mlflow.log_param.
- Around line 367-376: The code currently drops an explicit insecure_tls=True
when a connection dict is provided without an insecure_tls key and uses a broad
except; update the logic in the block that sets insecure = insecure_tls or
bool(connection and connection.get("insecure_tls")) so that when insecure is
true but connection["insecure_tls"] is not present you still set
MLFLOW_TRACKING_INSECURE_TLS (replace the guard `if not connection:` with `if
not (connection and connection.get("insecure_tls")):`), and change the exception
catch around importing urllib3 from a broad `except Exception:` to `except
ImportError:`; this ensures MLFLOW_TRACKING_INSECURE_TLS is set when the
explicit insecure_tls param is passed and only catches the expected import
failure, and keep mlflow_connection_env(connection)/connection_to_env() behavior
unchanged.
- Around line 20-35: Handle Path.relative_to ValueError in both
_mlflow_artifact_subdir and the verbose-logging branch inside log_artifacts:
wrap the call to file_path.resolve().relative_to(artifact_root.resolve()) in a
try/except ValueError and on exception fall back to returning None (for
_mlflow_artifact_subdir) or using only the basename/p.name in the verbose branch
so files outside artifact_root are uploaded under the file basename instead of
aborting; keep other logic unchanged and reference _mlflow_artifact_subdir and
the verbose-logging code path in log_artifacts when applying the change.
In `@projects/caliper/engine/file_export/mlflow_secrets.py`:
- Around line 128-149: The context manager mlflow_connection_env mutates
process-wide os.environ and is not safe to nest or run concurrently; update its
docstring to explicitly state "process-global; not safe to nest or run
concurrently" and then add a simple re-entrancy/concurrency guard around the
save/clear/apply/restore sequence (e.g., use a module-level threading.Lock or
asyncio.Lock) to prevent simultaneous entries; ensure the guard surrounds the
entire try/finally region that uses _ENV_KEYS and connection_to_env so restore
always runs under the same lock.
In `@projects/caliper/engine/file_export/runner.py`:
- Around line 14-27: The current apply_redaction_policy uses fnmatch which
doesn't support ** or path-aware matching; replace the fnmatch.fnmatch checks
with Path.match (e.g., use p.match(g) or pathlib.PurePath.match) so
exclude_globs like "**/secrets/*.yaml" and directory-aware patterns behave as
expected, and remove or relax the p.is_file() gating (since callers already
filter files) so non-file paths are not silently dropped; update the logic
around exclude (the exclude_globs lookup) accordingly and keep the rest of
apply_redaction_policy unchanged.
In `@projects/caliper/engine/file_export/s3.py`:
- Around line 34-42: The loop that builds tasks currently skips non-files and
missing paths silently (using p.is_file()) and will raise ValueError when
fp.relative_to(root) if a file resolves outside the source; update the logic in
the function that iterates over paths to (1) validate each input path p and if
it does not exist or is not a file either recurse into directories (e.g., walk
contents) or explicitly log/raise an error so callers see misconfiguration
instead of a successful count, (2) wrap the fp.relative_to(root) call (or
pre-check with Path.resolve().is_relative_to or equivalent) to catch ValueError
and log/raise a clear error when a resolved file is outside the source root, and
(3) ensure tasks only contains legitimate (Path, key) entries while adding
descriptive logs that include the offending p, root, and prefix to aid debugging
(references: paths loop, p.is_file(), fp.relative_to(root), tasks, prefix,
root).
In `@projects/caliper/orchestration/export.py`:
- Around line 30-32: The function run_from_orchestration_config currently
declares -> int but actually returns parsed YAML (yaml.safe_load(...)) on
success and raises RuntimeError on error; update the return type to reflect the
parsed YAML (e.g., dict[str, Any] | list[Any] | None) so callers get the correct
type, and simplify the call from yaml.safe_load(f.read()) to yaml.safe_load(f);
keep the RuntimeError behavior unchanged if you prefer raising instead of
returning an exit code.
- Around line 30-32: The function run_from_orchestration_config currently
assumes caliper_cfg is a dict and directly indexes caliper_cfg["export"], which
causes a TypeError when caliper_cfg is None; update
run_from_orchestration_config to explicitly validate caliper_cfg at the top
(reject None) with a clear error path: if caliper_cfg is None (or not a dict),
log an explanatory message and raise a ValueError (or TypeError per project
conventions) before any indexing, or change the parameter type to require
dict[str, Any] so callers cannot pass None; ensure the check occurs before any
use of caliper_cfg["export"] to preserve the documented dual-shape acceptance.
In `@projects/skeleton/orchestration/ci.py`:
- Around line 55-60: The finally block in test() can let exceptions from
_caliper_export_at_end() override failures from test_skeleton.test(); change
test to call _caliper_export_at_end() inside a try/except that catches and logs
any export errors (but does not re-raise them), while preserving and
returning/propagating the original result/exception from test_skeleton.test();
specifically, keep invoking test_skeleton.test() as before and wrap
_caliper_export_at_end() so export failures are logged (include context) and
swallowed so they cannot mask the primary test_skeleton.test() exception or
return value.
In `@projects/skeleton/orchestration/config.yaml`:
- Around line 17-39: The config currently sets caliper.export.dry_run: true
which prevents MLflow uploads in CI; update the caliper.export.dry_run value to
false when you’re ready to enable real exports and add a clear rollout note in
the deployment/rollout plan indicating the switch and who owns it; locate the
dry_run key under the caliper -> export -> dry_run block in the config and
change it to false (or make it driven by an env/CI variable) and ensure CI docs
or a README mention toggling this flag as part of the rollout checklist.
In `@specs/009-artifact-post-processing/contracts/cli.md`:
- Around line 8-14: Clarify and fix the CLI spec wording: decide whether the
artifacts export subcommand may load a project plugin per FR-017 and then update
the `--plugin` / `--base-dir` applicability text accordingly—either state
explicitly that `artifacts export` operates purely on a provided `--from PATH`
with no plugin resolution, or mark `--plugin MODULE` as allowed/conditional for
`artifacts export` so the backend can load the plugin to resolve "designated by
the project plugin" paths; adjust the sentence "Not used when no plugin is
loaded (e.g. `artifacts export`)" and the `--base-dir` conditional rules to
reflect the chosen behavior and reference `--plugin`, `--base-dir`,
`--postprocess-config`, and the `artifacts export` / `artifacts export`
subcommand semantics per FR-017.
- Around line 80-89: Add explicit documentation for the --continue-on-partial
flag referenced in the CLI contract: update the artifacts export options table
to include a row for `--continue-on-partial` describing its behavior (e.g.,
"when set, continue processing other backends if one backend fails; exit code
reflects overall status unless set") or, if semantics are undecided, add the row
with a "TBD" note specifying partial-failure policy to be finalized; ensure the
flag name `--continue-on-partial` appears verbatim so readers can find it and
that the table row references its interaction with `--backend` and exit-code
policy mentioned elsewhere.
In `@specs/009-artifact-post-processing/research.md`:
- Line 68: The markdown on the decision line has unbalanced emphasis markers
around the `--reports a,b,c` token; replace the malformed `**`--reports
a,b,c`**** list**` segment with a correctly balanced emphasis pattern (for
example a bold phrase containing the inline code: "**`--reports a,b,c` list**"
or simply "`--reports a,b,c` list") and ensure other tokens mentioned (like
`--report-group <id>`, `--visualize-config`, `--include-label`,
`--exclude-label`) remain unchanged.
In `@specs/009-artifact-post-processing/spec.md`:
- Around line 23-29: The two session blocks are out of chronological order:
"Session 2026-04-26" appears before "Session 2026-04-25"; reorder them so
sessions are ascending by date by swapping the "Session 2026-04-26" block (the
paragraph that starts "Q: How does manifest-only plugin discovery...") with the
"Session 2026-04-25" block (the paragraph that starts "Q: Should the engine
support exporting file artifacts..."), ensuring the headings and their full
paragraphs move together and formatting/spacing remains consistent in
specs/009-artifact-post-processing/spec.md.
In `@specs/009-artifact-post-processing/tasks.md`:
- Line 7: The spec file contains a developer-local absolute path in the Input
line which leaks a user-specific filesystem path; update the text in
specs/009-artifact-post-processing/tasks.md to use a repo-relative path instead
(e.g. change "`Design documents from
`/var/home/kpouget/openshift/forge/specs/009-artifact-post-processing/`" to
"`Design documents from `specs/009-artifact-post-processing/`") so the spec is
reproducible and not tied to a single contributor's workstation.
---
Nitpick comments:
In `@projects/caliper/cli/main.py`:
- Line 40: The CLI accepts arbitrary strings for the --backend option and
forwards them to run_file_export where unknown values are later marked
"skipped"; change validation at the CLI level by replacing the current
click.option("--backend", multiple=True, type=str, ...) with a constrained
choice (e.g., click.Choice(["s3","mlflow"], case_sensitive=False)) or add an
explicit validation check right after parsing that inspects the backends
variable and calls _exit_with_help on any unsupported value; reference the
--backend option, the backends variable in main.py, and
run_file_export/_exit_with_help so you validate/reject typos immediately instead
of deferring to run_file_export.
In `@projects/caliper/engine/file_export/mlflow_backend.py`:
- Around line 381-388: The code silently drops non-file entries when building
file_paths with [p for p in paths if p.is_file()], which hides missing files or
directories; update the logic around the paths -> file_paths filtering in
mlflow_backend.py (the variables/functions to inspect: paths, file_paths,
_parallel_workers, upload_workers, logger, verbose) to detect which entries were
filtered out and emit a logger.warning listing those dropped entries (include
the path and reason such as "missing" or "not a file"); when verbose is True
include the full list in the warning (otherwise a short summary/count) so
uploads are diagnosable.
- Around line 394-426: The two identical branches around mlflow.start_run should
be collapsed by preparing a single start_kw dict that conditionally sets
start_kw["run_id"]=run_id if run_id is provided else
start_kw["run_name"]=run_name (or leaves keys out when None), then call with
mlflow.start_run(**start_kw) once and execute the common body (calling
mlflow.active_run().info.run_id to set rid, _apply_run_metadata(effective_meta),
_apply_log_model(artifact_root, effective_meta, verbose),
_upload_mlflow_files_parallel(..., run_id=rid, ...), and meta =
_capture_mlflow_run_metadata(mlflow.get_tracking_uri() or "")) to avoid
duplicating logic and risk of drift.
In `@projects/caliper/engine/file_export/mlflow_config.py`:
- Around line 116-123: The validation for log_model.flavor and log_model.path in
mlflow_config.py currently coerces non-strings via str(...) which masks type
errors; update the checks to first ensure the values are instances of str and
then that they are non-empty (e.g., if not isinstance(val, str) or val.strip()
== ""), or alternatively move the existing loop that enforces string types (the
keys tuple check) to run before the "required" presence/emptiness checks so that
functions using log_model (the flavor/path checks) will fail with a clear
TypeError for wrong types and a separate error for empty strings; reference the
existing keys tuple and the flavor/path presence checks to locate where to
change.
In `@projects/caliper/engine/file_export/runner.py`:
- Around line 68-72: The loop over backends in runner.py currently appends
FileExportBackendResult with status "skipped" for unknown backend names; change
this to status "failure" (keeping detail="unknown backend") so
operator-configuration errors are reported as failures, matching the S3
ValueError path behavior and making FileExportBackendResult
aggregation/exit-code logic unambiguous; update the branch that checks b not in
("s3", "mlflow") to append status="failure" instead of "skipped".
In `@projects/caliper/engine/file_export/s3.py`:
- Around line 41-60: Normalize and validate the prefix once up front (e.g.,
compute prefix_normalized = prefix.strip('/') and treat empty string specially)
and then use that normalized value everywhere: build keys using
prefix_normalized (if non-empty, join as "{prefix_normalized}/{rel.as_posix()}",
otherwise just rel.as_posix()) in the code that creates tasks and in the return
value (return "s3://{bucket}/{prefix_normalized}" only when prefix_normalized is
non-empty, otherwise return "s3://{bucket}"); update any references to prefix in
the upload key construction and the final return, leaving helper _upload and
tasks logic unchanged.
In `@projects/caliper/engine/model.py`:
- Line 1: The module docstring and contained dataclasses are specific to the
file export/artifact upload pipeline but live in
projects/caliper/engine/model.py; either broaden the docstring and keep the
types here (if this truly is the engine-wide model) or move/rename to reflect
narrow scope — specifically, move the file-export dataclasses currently
re-exported from file_export/types.py into that package (or rename model.py to
file_export_models.py) and update all imports/re-exports (references: the two
file-export dataclasses and the re-export in file_export/types.py) so the module
name matches its responsibility and the top-level docstring accurately describes
the content.
In `@projects/caliper/orchestration/export_config.py`:
- Line 49: The top-level orchestration Pydantic models
(CaliperExportBackendMlflow, CaliperExportBackends,
CaliperOrchestrationExportConfig) are using model_config =
ConfigDict(extra="ignore") which silently drops unknown YAML keys; change these
top-level models to use extra="forbid" instead (adjust the model_config
assignment used by those classes or override each class' Config/model_config) so
unknown keys raise validation errors early — note the nested vault/secrets
models already use extra="forbid", so mirror that for the top-level
orchestration configs referenced by model_config on lines around the
CaliperExportBackendMlflow, CaliperExportBackends and
CaliperOrchestrationExportConfig definitions.
- Around line 90-98: The validator _at_least_one_backend currently has to guard
for backend.mlflow being None because the field mlflow is declared nullable; to
make errors clearer, change the CaliperExportBackendMlflow field to be
non-nullable (remove "| None" and the default None) so callers must supply
backend.mlflow, then simplify _at_least_one_backend to check
backend.mlflow.enabled directly (remove the b.mlflow is not None branch) and
raise a single ValueError referencing "backend.mlflow.enabled: true" when not
enabled; if you prefer to keep mlflow nullable instead, modify the validator to
raise a distinct message when b.mlflow is None (e.g., "backend.mlflow is
missing; add backend.mlflow.enabled: true") so users know the nesting is
required.
In `@projects/core/notifications/send.py`:
- Around line 345-350: Replace the print-based error handling around
json.loads(os.environ["JOB_SPEC"]) with the module logger: catch the exception
and call logger.exception or logger.error (including the exception) with a
clearer diagnostic like "Failed to parse JOB_SPEC environment variable" and
include a safely redacted or truncated preview of os.environ["JOB_SPEC"] (or
note redaction if sensitive) so the stacktrace and context are routed through
the module logger; update the try/except block that assigns job_spec to use
logger.exception(logger.error) instead of print and re-raise the exception as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 262b08b0-4167-456a-a46e-91fe6e25d9fa
📒 Files selected for processing (40)
bin/caliperdocs/caliper/mlflow-config.example.yamlprojects/caliper/README.mdprojects/caliper/__init__.pyprojects/caliper/cli/__init__.pyprojects/caliper/cli/main.pyprojects/caliper/engine/__init__.pyprojects/caliper/engine/file_export/__init__.pyprojects/caliper/engine/file_export/artifacts_export_run.pyprojects/caliper/engine/file_export/mlflow_backend.pyprojects/caliper/engine/file_export/mlflow_config.pyprojects/caliper/engine/file_export/mlflow_secrets.pyprojects/caliper/engine/file_export/runner.pyprojects/caliper/engine/file_export/s3.pyprojects/caliper/engine/file_export/types.pyprojects/caliper/engine/model.pyprojects/caliper/orchestration/__init__.pyprojects/caliper/orchestration/export.pyprojects/caliper/orchestration/export_config.pyprojects/core/ci_entrypoint/run_ci.pyprojects/core/library/ci.pyprojects/core/library/export.pyprojects/core/notifications/send.pyprojects/jump_ci/testing/runprojects/skeleton/orchestration/ci.pyprojects/skeleton/orchestration/config.yamlpyproject.tomlspecs/009-artifact-post-processing/checklists/requirements.mdspecs/009-artifact-post-processing/checklists/spec-quality.mdspecs/009-artifact-post-processing/contracts/cli.mdspecs/009-artifact-post-processing/contracts/kpi_record.mdspecs/009-artifact-post-processing/contracts/postprocess_manifest.mdspecs/009-artifact-post-processing/contracts/test_labels.mdspecs/009-artifact-post-processing/data-model.mdspecs/009-artifact-post-processing/plan.mdspecs/009-artifact-post-processing/quickstart.mdspecs/009-artifact-post-processing/research.mdspecs/009-artifact-post-processing/spec.mdspecs/009-artifact-post-processing/tasks.mdvaults/psap-forge-mlflow-export.yaml
✅ Files skipped from review due to trivial changes (16)
- projects/caliper/init.py
- projects/caliper/engine/file_export/init.py
- projects/core/ci_entrypoint/run_ci.py
- projects/caliper/engine/init.py
- projects/caliper/engine/file_export/types.py
- projects/core/library/ci.py
- docs/caliper/mlflow-config.example.yaml
- specs/009-artifact-post-processing/contracts/kpi_record.md
- specs/009-artifact-post-processing/contracts/postprocess_manifest.md
- vaults/psap-forge-mlflow-export.yaml
- specs/009-artifact-post-processing/checklists/requirements.md
- projects/caliper/cli/init.py
- bin/caliper
- projects/caliper/orchestration/init.py
- specs/009-artifact-post-processing/data-model.md
- projects/caliper/engine/file_export/artifacts_export_run.py
🚧 Files skipped from review as they are similar to previous changes (5)
- specs/009-artifact-post-processing/contracts/test_labels.md
- projects/caliper/README.md
- pyproject.toml
- specs/009-artifact-post-processing/quickstart.md
- specs/009-artifact-post-processing/plan.md
Comment on lines
+157
to
+171
| def run_cli() -> None: | ||
| try: | ||
| rv = main.main(standalone_mode=False, prog_name="caliper") | ||
| if isinstance(rv, int) and rv != 0: | ||
| sys.exit(rv) | ||
| except click.MissingParameter as exc: | ||
| msg = exc.format_message() | ||
| sub = getattr(exc, "ctx", None) | ||
| click.echo(f"Error: {msg}\n", err=True) | ||
| if sub is not None: | ||
| click.echo(sub.get_help(), err=True) | ||
| ec = getattr(exc, "exit_code", 2) | ||
| sys.exit(2 if ec is None else int(ec)) | ||
| except SystemExit: | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm there are no other tests/callers asserting this standalone_mode behaviour.
rg -nP --type=py -C3 'main\.main\(standalone_mode=False'
rg -nP --type=py -C3 'from projects\.caliper\.cli\.main'Repository: openshift-psap/forge
Length of output: 506
🏁 Script executed:
cat -n projects/caliper/cli/main.py | head -180Repository: openshift-psap/forge
Length of output: 6587
🏁 Script executed:
rg --type=py -B20 'def run_cli' projects/caliper/cli/main.pyRepository: openshift-psap/forge
Length of output: 696
🏁 Script executed:
rg --type=py 'import.*main' projects/caliper/cli/main.py | head -20Repository: openshift-psap/forge
Length of output: 46
🏁 Script executed:
fd -t py '.*test.*' | xargs grep -l 'run_cli\|standalone_mode' 2>/dev/null | head -10Repository: openshift-psap/forge
Length of output: 234
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "run_cli" 2>/dev/nullRepository: openshift-psap/forge
Length of output: 95
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "test.*caliper.*cli\|caliper.*cli.*test" 2>/dev/null | head -10Repository: openshift-psap/forge
Length of output: 46
🏁 Script executed:
rg --type=py "standalone_mode" -B2 -A2Repository: openshift-psap/forge
Length of output: 377
🏁 Script executed:
git log --oneline -n 20 -- projects/caliper/cli/main.py 2>/dev/null | head -20Repository: openshift-psap/forge
Length of output: 110
Incomplete Click error handling with standalone_mode=False — consider broadening exception catch or using Click's default mode.
run_cli() calls main.main(standalone_mode=False, prog_name="caliper") and only catches click.MissingParameter. With standalone_mode=False, other Click errors (BadParameter, UsageError, NoSuchOption, etc.) will bubble up as unhandled tracebacks instead of friendly usage messages. Either catch click.ClickException broadly or let Click's default standalone mode handle the UX.
Also, except SystemExit: raise on lines 170–171 is redundant—SystemExit propagates naturally when sys.exit() is called.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/caliper/cli/main.py` around lines 157 - 171, run_cli currently calls
main.main(standalone_mode=False, prog_name="caliper") but only catches
click.MissingParameter, so other Click errors (BadParameter, UsageError,
NoSuchOption, etc.) surface as tracebacks; broaden the handler to catch
click.ClickException (or click.exceptions.ClickException) instead of only
click.MissingParameter, echo the error message to stderr (click.echo(str(exc),
err=True)), print ctx.get_help() when exc has a ctx attribute, and exit using
exc.exit_code; also remove the redundant "except SystemExit: raise" block since
SystemExit will naturally propagate.
Comment on lines
+20
to
+35
| def _mlflow_artifact_subdir(artifact_root: Path, file_path: Path) -> str | None: | ||
| """ | ||
| Parent directory of ``file_path`` relative to ``artifact_root``, as a POSIX path. | ||
|
|
||
| MLflow ``log_artifact(local, artifact_path=...)`` places the file basename under that | ||
| subdirectory, preserving hierarchy when ``artifact_root`` is a directory. | ||
| """ | ||
| root = artifact_root.resolve() | ||
| fp = file_path.resolve() | ||
| if root.is_file(): | ||
| return None | ||
| rel = fp.relative_to(root) | ||
| parent = rel.parent | ||
| if parent == Path("."): | ||
| return None | ||
| return parent.as_posix() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python pathlib Path.relative_to walk_up parameter which version introduced
💡 Result:
The walk_up parameter for pathlib.Path.relative_to (and PurePath.relative_to) was introduced in Python 3.12.
Citations:
- 1: https://stackoverflow.com/questions/77880475/pathlibs-relative-to-doesnt-accept-walk-up-parameter
- 2: https://docs.python.org/3/library/pathlib.html?highlight=replace
- 3: https://docs.python.org/3/library/pathlib.html
- 4: https://docs.python.org/release/3.12.12/library/pathlib.html
- 5: https://docs.python.org/3/whatsnew/3.12.html
🏁 Script executed:
#!/bin/bash
# Find and read the mlflow_backend.py file
find . -type f -name "mlflow_backend.py" | head -1Repository: openshift-psap/forge
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Read the file with line numbers to see the actual code at the mentioned lines
FILE=$(find . -type f -name "mlflow_backend.py" | head -1)
if [ -n "$FILE" ]; then
wc -l "$FILE"
fiRepository: openshift-psap/forge
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Check Python version requirements
find . -type f \( -name "pyproject.toml" -o -name "setup.py" -o -name ".python-version" -o -name "setup.cfg" \) | head -5Repository: openshift-psap/forge
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Read lines 20-35 from the mlflow_backend.py file
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '20,35p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 767
🏁 Script executed:
#!/bin/bash
# Read lines 305-329 from the mlflow_backend.py file (the other location mentioned)
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '305,329p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Check the pyproject.toml for Python version requirements
cat pyproject.toml | head -50Repository: openshift-psap/forge
Length of output: 1313
🏁 Script executed:
#!/bin/bash
# Search for callers of _upload_mlflow_files_parallel
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
rg "_upload_mlflow_files_parallel" "$FILE" -nRepository: openshift-psap/forge
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Search for callers in the codebase
rg "_upload_mlflow_files_parallel" --type py -nRepository: openshift-psap/forge
Length of output: 367
🏁 Script executed:
#!/bin/bash
# Read around line 399 to see how _upload_mlflow_files_parallel is called
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '390,410p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 1125
🏁 Script executed:
#!/bin/bash
# Read around line 417 to see the second call
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '408,428p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 1152
🏁 Script executed:
#!/bin/bash
# Look for any validation/filtering of file_paths before calling the function
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '370,405p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 1761
🏁 Script executed:
#!/bin/bash
# Read more context around the file_paths filtering to understand where paths come from
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '340,385p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 1920
🏁 Script executed:
#!/bin/bash
# Check if there's a function signature for log_artifacts to see where paths originate
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
rg "def log_artifacts" -n -A 10Repository: openshift-psap/forge
Length of output: 996
🏁 Script executed:
#!/bin/bash
# Check the full context of log_artifacts function parameters
FILE="./projects/caliper/engine/file_export/mlflow_backend.py"
sed -n '342,383p' "$FILE" | cat -nRepository: openshift-psap/forge
Length of output: 1833
Path.relative_to raises ValueError when a file isn't under artifact_root.
Both _mlflow_artifact_subdir (line 31) and the verbose-logging branch (line 326) call relative_to without catching ValueError. The log_artifacts function only pre-filters paths by is_file() but does not validate they are under artifact_root. If any path in file_paths resolves outside artifact_root, the entire MLflow upload aborts with a bare traceback instead of gracefully falling back to basename-only placement.
The suggested walk_up=True parameter is available only in Python 3.12+, but the project supports Python 3.11. A more portable fix is to use try/except ValueError that falls back to None or p.name for both code paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/caliper/engine/file_export/mlflow_backend.py` around lines 20 - 35,
Handle Path.relative_to ValueError in both _mlflow_artifact_subdir and the
verbose-logging branch inside log_artifacts: wrap the call to
file_path.resolve().relative_to(artifact_root.resolve()) in a try/except
ValueError and on exception fall back to returning None (for
_mlflow_artifact_subdir) or using only the basename/p.name in the verbose branch
so files outside artifact_root are uploaded under the file basename instead of
aborting; keep other logic unchanged and reference _mlflow_artifact_subdir and
the verbose-logging code path in log_artifacts when applying the change.
Contributor
Author
|
/test fournos skeleton collect_cluster_info |
kpouget
force-pushed
the
009-artifact-post-processing
branch
from
April 27, 2026 11:28
fc12c5e to
2299b0e
Compare
This was referenced Jun 23, 2026
This was referenced Jul 1, 2026
This was referenced Aug 2, 2026
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
1 participant
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 CodeRabbit
New Features
Documentation
Chores