[caliper] Refactor the post-processing file - #148
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 Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesKPI formatting and postprocessing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Postprocess
participant CaliperInvocation
participant CaliperCLI
participant StatusFile
participant StepLog
Postprocess->>CaliperInvocation: invoke workflow step
CaliperInvocation->>CaliperCLI: execute generated command
CaliperCLI->>StatusFile: write YAML status
CaliperCLI->>StepLog: emit captured output
CaliperInvocation->>StatusFile: parse completion status
CaliperInvocation->>Postprocess: return step result and failure state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
/test fournos skeleton |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos skeleton |
🔴 Execution of
|
🔴 Submission of
|
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
projects/caliper/orchestration/caliper_invocation.py (2)
62-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
plugin/model/plugin_moduleparameters.These runners now shell out to the CLI, so the in-process plugin and model objects are never touched (and
plugin/modelare untyped). Dropping them from the signatures — and from the three call sites inprojects/caliper/orchestration/postprocess.py(Lines 606, 644, 676) — would remove the misleading contract and the plugin-loading coupling.run_analyse_kpisalso takesplugin_modulewithout using it.Also applies to: 148-157, 224-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/orchestration/caliper_invocation.py` around lines 62 - 71, Remove the unused plugin, model, and plugin_module parameters from run_artifacts_to_kpis, run_analyse_kpis, and the other affected runner signature near the referenced definitions. Update all three corresponding call sites in postprocess.py to stop passing these arguments, preserving the remaining parameter order and CLI-based execution.
603-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
run_s3_export_stepis unused —postprocess.pystill inlines the S3 export flow.
projects/caliper/orchestration/postprocess.pyimportsbuild_s3_export_commandand_execute_caliper_commanddirectly and duplicates this logic in_run_s3_export_step(Lines 844-898), which also means the export step keeps a non-temp status file inoutput_dirwhile this runner uses a temp file. Either delegate to this runner like the other steps, or drop it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/orchestration/caliper_invocation.py` around lines 603 - 611, The new run_s3_export_step function is unused because postprocess.py still implements the export flow inline. Update postprocess.py’s _run_s3_export_step to delegate to run_s3_export_step, removing its direct build_s3_export_command and _execute_caliper_command usage, or remove run_s3_export_step if delegation is not intended; ensure the active path uses the runner’s temporary status-file behavior.projects/caliper/orchestration/postprocess_logging.py (1)
127-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
yamland_get_next_step_indeximports to module top.
step_loggingonly uses standard-library imports, so these function-local imports can be hoisted instead of obscuring dependencies. This applies to theimport yamlin_write_step_outputand thefrom projects.caliper.orchestration.step_logging import _get_next_step_indexin_execute_caliper_command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/orchestration/postprocess_logging.py` at line 127, Move the yaml import and the _get_next_step_index import from their local locations in _write_step_output and _execute_caliper_command to the module-level imports, preserving their existing usage and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 68-134: Update the KPI formatting flow around kpi_functions,
final_value, and kpi_record so `_kpi_is_2d` is resolved before transforming
raw_value and serves as the source of truth when metadata exists. Apply the
tuple-pair structural transform only for confirmed 2D KPIs, and use one
consistent fallback heuristic when metadata is absent so `is_2d` always matches
value shape. Preserve the original list-of-tuples representation required by
read_kpis_from_file round-tripping instead of exposing the structured dictionary
to flattened records.
- Around line 41-61: Update the label merge in the KPI formatting loop to union
labels from every KPI sharing a run_id instead of only assigning when
test_data["labels"] is empty. Preserve the existing exclusion of
"higher_is_better", and ensure later KPI labels are merged into the accumulated
labels without discarding previously collected values.
In `@projects/caliper/orchestration/caliper_invocation.py`:
- Around line 429-435: Update all four failure-result branches in the relevant
invocation flow, including the branch returning the shown dictionary and the
branches around the other reported locations, to use the status value "failed"
instead of "failure". Preserve the existing failure details, exit code,
timestamp, and log-file fields.
- Around line 429-435: Normalize failure results to status "failed" across the
failure and exception branches in run_caliper_step, run_visualize_step,
run_s3_import_step, and run_s3_export_step, replacing "failure" wherever used.
In postprocess.py, update the s3 import handling to set self.s3_import_failed
when the returned success boolean is False, in addition to existing status
checks; apply these changes at
projects/caliper/orchestration/caliper_invocation.py:429-435 and
projects/caliper/orchestration/postprocess.py:703-714.
- Around line 107-121: Update the success checks in the KPI-family runners
around _execute_caliper_command—run_kpi_generate, run_artifacts_to_ai_data,
run_kpis_to_csv, and run_analyse_kpis—to require both result.returncode == 0 and
status_data.get("success"). Preserve the existing failure handling and
conversion behavior when either condition is false.
In `@projects/caliper/orchestration/postprocess_logging.py`:
- Around line 159-165: Update the status logging block around the status_data
handling to avoid dumping every YAML key/value verbatim. Log only an explicit
allowlist of known-safe fields such as success, error, exported_path, and
counts, or redact all unknown keys before logging; ensure secret-bearing values
and arbitrary error text are never persisted under ARTIFACT_DIR.
- Around line 209-214: Add a configurable timeout to the subprocess.run call in
the postprocess execution flow, and catch subprocess.TimeoutExpired to mark the
command as failed while preserving normal downstream status_data handling. Use
the existing configuration mechanism or introduce the nearest appropriate
timeout setting, without changing command construction or shell behavior.
---
Nitpick comments:
In `@projects/caliper/orchestration/caliper_invocation.py`:
- Around line 62-71: Remove the unused plugin, model, and plugin_module
parameters from run_artifacts_to_kpis, run_analyse_kpis, and the other affected
runner signature near the referenced definitions. Update all three corresponding
call sites in postprocess.py to stop passing these arguments, preserving the
remaining parameter order and CLI-based execution.
- Around line 603-611: The new run_s3_export_step function is unused because
postprocess.py still implements the export flow inline. Update postprocess.py’s
_run_s3_export_step to delegate to run_s3_export_step, removing its direct
build_s3_export_command and _execute_caliper_command usage, or remove
run_s3_export_step if delegation is not intended; ensure the active path uses
the runner’s temporary status-file behavior.
In `@projects/caliper/orchestration/postprocess_logging.py`:
- Line 127: Move the yaml import and the _get_next_step_index import from their
local locations in _write_step_output and _execute_caliper_command to the
module-level imports, preserving their existing usage and behavior.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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 Plus
Run ID: e038e7eb-d758-408d-aff9-fed9e8316d26
📒 Files selected for processing (6)
projects/caliper/cli/commands.pyprojects/caliper/engine/kpi/format.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_logging.py
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/caliper/orchestration/postprocess.py (1)
780-921: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winS3 export success check ignores the subprocess exit code — stale status file can mask a failed run.
status_file = output_dir / "s3_export_status.yaml"(line 848) is a fixed path reused across runs, not a per-invocation tempfile. Line 875 decides success purely fromstatus_data.get("success"), without also requiringresult.returncode == 0(unlike every other runner incaliper_invocation.py, which was already fixed to check both). If thecaliper s3-exportsubprocess crashes before rewriting the status file, a stalesuccess: truefrom a previous run would be read as success, silently masking a failed export.🐛 Minimal fix
- if status_data.get("success"): + if result.returncode == 0 and status_data.get("success"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/orchestration/postprocess.py` around lines 780 - 921, Update the success decision in _run_s3_export_step after _execute_caliper_command to require both result.returncode == 0 and status_data.get("success"). Treat any nonzero subprocess exit as failed, even when the status file reports success, while preserving the existing success and failure result handling.
🧹 Nitpick comments (2)
projects/caliper/engine/kpi/format.py (1)
121-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
namefrom the docstring's first line.
func.__doc__includes the full, indented multi-line docstring, sonamecan leak whole paragraphs plus whitespace. Use the summary line only.♻️ Proposed refactor
"name": ( - func.__doc__.replace(" KPI.", "") + func.__doc__.strip().splitlines()[0].replace(" KPI.", "").strip() if func.__doc__ else kpi_id.replace("_", " ").title() ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/format.py` around lines 121 - 125, Update the name derivation in the KPI formatting logic to use only the first line of func.__doc__, removing indentation and the existing “ KPI.” suffix as needed; preserve the kpi_id-based fallback when no docstring exists.projects/caliper/orchestration/postprocess.py (1)
20-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winS3 export step wasn't migrated to the new shared runner, leaving
run_s3_export_stepunused andpostprocess.pywith a duplicate (buggy) inline implementation. Every other step (parse, visualize, KPI, AI data, CSV, analyze, S3 import) was migrated to thecaliper_invocation.pyrunners in this refactor; S3 export is the one exception.
projects/caliper/orchestration/postprocess.py#L20-L29: importrun_s3_export_stepalongside the other runners.projects/caliper/orchestration/caliper_invocation.py#L664-L742: currently dead code; wire it up as the sole implementation for S3 export.projects/caliper/orchestration/postprocess.py#L780-L921: replace the inline_execute_caliper_command/build_s3_export_commandlogic with a call torun_s3_export_step(...), preserving theexported_path/uploaded_files/total_filesfields currently read fromstatus_data(either by extending the runner's success payload or extracting them fromstep_resultafter the call) so downstream report generation keeps working.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/orchestration/postprocess.py` around lines 20 - 29, Import run_s3_export_step in postprocess.py alongside the other shared runners. Use the existing run_s3_export_step implementation in caliper_invocation.py as the sole S3 export path, replacing the inline _execute_caliper_command/build_s3_export_command logic while preserving exported_path, uploaded_files, and total_files for downstream report generation. Update projects/caliper/orchestration/postprocess.py lines 20-29, 780-921 and wire the runner at projects/caliper/orchestration/caliper_invocation.py lines 664-742; no direct change is needed in the runner unless required to expose the preserved status fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 79-85: Update the is_2d expression in the KPI formatting logic to
always produce a boolean by wrapping the existing condition in bool(...). Remove
the redundant len(raw_value) > 0 check while preserving the current list,
non-empty, nested-pair validation and the kpi_record["is_2d"] output contract.
In `@projects/caliper/engine/kpi/generate.py`:
- Line 21: Update the KPI file-reading paths in kpi_csv_export and kpi_view to
use read_kpis_from_file() instead of parsing line-delimited JSON directly,
preserving support for both hierarchical JSON and existing KPI file formats.
---
Outside diff comments:
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 780-921: Update the success decision in _run_s3_export_step after
_execute_caliper_command to require both result.returncode == 0 and
status_data.get("success"). Treat any nonzero subprocess exit as failed, even
when the status file reports success, while preserving the existing success and
failure result handling.
---
Nitpick comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 121-125: Update the name derivation in the KPI formatting logic to
use only the first line of func.__doc__, removing indentation and the existing “
KPI.” suffix as needed; preserve the kpi_id-based fallback when no docstring
exists.
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 20-29: Import run_s3_export_step in postprocess.py alongside the
other shared runners. Use the existing run_s3_export_step implementation in
caliper_invocation.py as the sole S3 export path, replacing the inline
_execute_caliper_command/build_s3_export_command logic while preserving
exported_path, uploaded_files, and total_files for downstream report generation.
Update projects/caliper/orchestration/postprocess.py lines 20-29, 780-921 and
wire the runner at projects/caliper/orchestration/caliper_invocation.py lines
664-742; no direct change is needed in the runner unless required to expose the
preserved status fields.
🪄 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 Plus
Run ID: ed04d42c-7a0d-411f-bd65-ef893cf127f3
📒 Files selected for processing (6)
projects/caliper/cli/commands.pyprojects/caliper/engine/kpi/format.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_logging.py
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
projects/caliper/engine/kpi/format.py (1)
79-85: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
is_2dcan still serialize as a non-boolean.The
andchain short-circuits on falsyraw_value(e.g.[],None,0), sokpi_record["is_2d"]at Line 113 emits[]/nullinstead offalse. Line 82 is also redundant with Line 81.🐛 Proposed fix
- is_2d = ( - isinstance(raw_value, list) - and raw_value - and len(raw_value) > 0 - and isinstance(raw_value[0], (list, tuple)) - and len(raw_value[0]) == 2 - ) + is_2d = bool( + isinstance(raw_value, list) + and raw_value + and isinstance(raw_value[0], (list, tuple)) + and len(raw_value[0]) == 2 + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/format.py` around lines 79 - 85, Update the is_2d expression in the KPI formatting logic to always produce a boolean: remove the redundant length check and explicitly normalize the remaining condition with a boolean conversion. Preserve the existing 2D detection criteria for non-empty list values whose first element is a two-item list or tuple.projects/caliper/engine/kpi/generate.py (1)
21-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefault flip to
hierarchicalbreaks readers that parse KPI files as JSONL.Any consumer still doing per-line
json.loads(e.g. the CSV export / view paths) will fail or silently read nothing once the default output is schema-v2 JSON. Route those readers throughread_kpis_from_file().#!/bin/bash # Find KPI file readers that bypass read_kpis_from_file rg -nP --type=py -C4 'json\.loads\(line|splitlines\(\)' projects/caliper rg -nP --type=py -C3 '\bread_kpis_from_file\s*\(' projects🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/generate.py` at line 21, Update KPI file-reading paths that parse files line by line with json.loads(line) or splitlines() to use read_kpis_from_file() instead, including CSV export and view paths. Preserve the existing downstream behavior while ensuring readers handle the default hierarchical schema-v2 output.
🧹 Nitpick comments (1)
projects/caliper/orchestration/caliper_invocation.py (1)
123-437: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider collapsing the four near-identical KPI runners.
run_artifacts_to_kpis,run_artifacts_to_ai_data,run_kpis_to_csv, andrun_analyse_kpisshare the same skeleton (enabled check → build command →_execute_caliper_command→ unlink status file → success/failure dict → exception handler). A small helper taking(step_name, status_filename, command_builder, output_file)would remove ~200 duplicated lines and keep the failure/log_filecontract consistent across steps. Also worth typingpostprocess_configonrun_artifacts_to_ai_data(Line 210) like its siblings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/orchestration/caliper_invocation.py` around lines 123 - 437, Collapse the shared execution flow of run_artifacts_to_kpis, run_artifacts_to_ai_data, run_kpis_to_csv, and run_analyse_kpis into a helper accepting the step name, status filename, command builder, and output file, while preserving each runner’s enablement checks, command-specific arguments, result fields, failure handling, and log_file contract. Update run_artifacts_to_ai_data to use the same CaliperOrchestrationPostprocessConfig type as its sibling runners.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 228-267: Update the JSON parsing flow around the hierarchical
flattening branch so records are accumulated in a temporary list and merged into
kpis only after the entire branch succeeds; ensure the JSONL fallback starts
without any partially appended hierarchical records. When flattening 2D KPI
records, convert the structured value containing data_points and count back to
the schema-v1 list-of-pairs representation expected by the JSONL writer, while
preserving scalar values unchanged.
In `@projects/caliper/orchestration/caliper_invocation.py`:
- Around line 261-268: Update the success result returned by the invocation flow
around the status success branch to include the AI data directory under the
exact key ai_data_dir expected by postprocess.py, while preserving output_file
for report links and all existing success metadata.
In `@projects/caliper/orchestration/postprocess_logging.py`:
- Around line 143-161: Update the status-data handling around yaml.safe_load in
the postprocessing function to normalize every non-mapping payload, including
lists, strings, and integers, into the existing failure dictionary before
returning or logging it. Preserve valid mapping results and the current
empty/invalid YAML and exception error behavior so callers in
caliper_invocation.py can safely use status_data.get(...).
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 20-29: Import run_s3_export_step from caliper_invocation and
update _run_s3_export_step to only collect the required paths before delegating
to that shared runner. Remove its inline _execute_caliper_command flow and rely
on run_s3_export_step’s return-code-aware success handling.
---
Duplicate comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 79-85: Update the is_2d expression in the KPI formatting logic to
always produce a boolean: remove the redundant length check and explicitly
normalize the remaining condition with a boolean conversion. Preserve the
existing 2D detection criteria for non-empty list values whose first element is
a two-item list or tuple.
In `@projects/caliper/engine/kpi/generate.py`:
- Line 21: Update KPI file-reading paths that parse files line by line with
json.loads(line) or splitlines() to use read_kpis_from_file() instead, including
CSV export and view paths. Preserve the existing downstream behavior while
ensuring readers handle the default hierarchical schema-v2 output.
---
Nitpick comments:
In `@projects/caliper/orchestration/caliper_invocation.py`:
- Around line 123-437: Collapse the shared execution flow of
run_artifacts_to_kpis, run_artifacts_to_ai_data, run_kpis_to_csv, and
run_analyse_kpis into a helper accepting the step name, status filename, command
builder, and output file, while preserving each runner’s enablement checks,
command-specific arguments, result fields, failure handling, and log_file
contract. Update run_artifacts_to_ai_data to use the same
CaliperOrchestrationPostprocessConfig type as its sibling runners.
🪄 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 Plus
Run ID: e5fa56e1-97f4-4391-a23b-6ac644540e1d
📒 Files selected for processing (6)
projects/caliper/cli/commands.pyprojects/caliper/engine/kpi/format.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_logging.py
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
projects/caliper/engine/kpi/generate.py (1)
21-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefault
"hierarchical"changes the on-disk format for all existing callers.Any consumer that parses the generated file as line-delimited JSON (e.g.
kpi_csv_export,kpi_view) breaks unless it switches toread_kpis_from_file. Either migrate those readers or keep"jsonl"as the default until they are updated.#!/bin/bash # Find KPI file readers that still assume JSONL rg -nP --type=py -C4 'read_kpis_from_file|json\.loads\(line' projects/caliperAlso applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/generate.py` at line 21, Change the default format_type in the KPI generation configuration to "jsonl" to preserve the existing on-disk format for current callers. Keep the hierarchical format available when explicitly requested, and do not alter readers such as kpi_csv_export or kpi_view unless they are separately migrated to read via read_kpis_from_file.projects/caliper/engine/kpi/format.py (1)
79-85: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
is_2dcan be a list rather than a bool.When
raw_valueis[], theandchain short-circuits onraw_valueand yields[], sokpi_record["is_2d"](Line 113) serializes as[]instead offalse. Alsolen(raw_value) > 0is redundant with the truthiness check.🐛 Proposed fix
- is_2d = ( - isinstance(raw_value, list) - and raw_value - and len(raw_value) > 0 - and isinstance(raw_value[0], (list, tuple)) - and len(raw_value[0]) == 2 - ) + is_2d = bool( + isinstance(raw_value, list) + and raw_value + and isinstance(raw_value[0], (list, tuple)) + and len(raw_value[0]) == 2 + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/format.py` around lines 79 - 85, Update the is_2d expression in the raw_value classification logic to always produce a boolean, including false for an empty list. Remove the redundant len(raw_value) > 0 check while preserving the existing list, first-element shape, and two-item validation.
🧹 Nitpick comments (2)
projects/caliper/engine/kpi/format.py (2)
210-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify: the
elsebranch duplicates the dict fallback return.Both the non-dict path and the "other dict structures" path return
raw_valueunchanged, so the trailingelseis redundant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/format.py` around lines 210 - 234, Simplify _convert_to_schema_v1_value by removing the redundant trailing else branch and returning raw_value once after the dict-specific handling. Preserve the existing data_points conversion and unchanged return behavior for all other dictionaries and scalar values.
183-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
format_typebefore creating directories.
output_file.parent.mkdir(...)runs before the unknown-format check, so an invalidformat_typestill leaves new directories behind.♻️ Suggested reordering
- output_file.parent.mkdir(parents=True, exist_ok=True) + if format_type not in ("hierarchical", "jsonl"): + raise ValueError(f"Unknown format type: {format_type}. Use 'hierarchical' or 'jsonl'") + + output_file.parent.mkdir(parents=True, exist_ok=True) if format_type == "hierarchical":🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/caliper/engine/kpi/format.py` around lines 183 - 207, Move validation of format_type ahead of output_file.parent.mkdir in the formatting function, rejecting values other than "hierarchical" and "jsonl" before any filesystem changes occur. Preserve the existing hierarchical and jsonl writing behavior after validation, and keep the unknown-format ValueError message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 255-305: The schema-v2 flattening path in the KPI formatting
function loses is_2d, name, help, x_unit, and y_unit metadata; preserve these
fields in each flattened record or explicitly document the intentional loss in
the function docstring. Also narrow the exception handling so only json.loads
parsing failures trigger the JSONL fallback; let ValueError and other structural
errors from _convert_to_schema_v1_value or flattening propagate instead of being
silently converted to an empty result.
In `@projects/caliper/orchestration/caliper_invocation.py`:
- Around line 665-673: Update run_s3_export_step to remove the unused required
output_dir parameter, then adjust its callers to match the revised signature,
including the new invocation in postprocess.py. Preserve all existing export
behavior and argument handling.
- Around line 298-304: The CSV runner’s disabled check and related configuration
access use the nonexistent kpi.csv path. Update the CSV export flow around the
visible disabled branch to use postprocess_config.kpi.kpis_to_csv, including its
enabled and output settings, while preserving the existing disabled response
behavior.
In `@projects/caliper/orchestration/postprocess.py`:
- Around line 837-847: The shared S3 export runner migration is incomplete and
breaks import/execution. In projects/caliper/orchestration/postprocess.py lines
837-847, pass output_dir, unpack the runner’s (success, step_result) result, and
branch on success; in lines 20-29, import run_s3_export_step and remove the
unused _execute_caliper_command; in lines 862-869, remove the orphaned
exception-body references or restore a try/except around the delegation. In
projects/caliper/orchestration/caliper_invocation.py lines 665-673, remove the
required output_dir parameter or give it a default so the runner signature
matches its callers.
---
Duplicate comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 79-85: Update the is_2d expression in the raw_value classification
logic to always produce a boolean, including false for an empty list. Remove the
redundant len(raw_value) > 0 check while preserving the existing list,
first-element shape, and two-item validation.
In `@projects/caliper/engine/kpi/generate.py`:
- Line 21: Change the default format_type in the KPI generation configuration to
"jsonl" to preserve the existing on-disk format for current callers. Keep the
hierarchical format available when explicitly requested, and do not alter
readers such as kpi_csv_export or kpi_view unless they are separately migrated
to read via read_kpis_from_file.
---
Nitpick comments:
In `@projects/caliper/engine/kpi/format.py`:
- Around line 210-234: Simplify _convert_to_schema_v1_value by removing the
redundant trailing else branch and returning raw_value once after the
dict-specific handling. Preserve the existing data_points conversion and
unchanged return behavior for all other dictionaries and scalar values.
- Around line 183-207: Move validation of format_type ahead of
output_file.parent.mkdir in the formatting function, rejecting values other than
"hierarchical" and "jsonl" before any filesystem changes occur. Preserve the
existing hierarchical and jsonl writing behavior after validation, and keep the
unknown-format ValueError message.
🪄 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 Plus
Run ID: 876d5ea7-266f-45e3-921b-aa747f0364e4
📒 Files selected for processing (6)
projects/caliper/cli/commands.pyprojects/caliper/engine/kpi/format.pyprojects/caliper/engine/kpi/generate.pyprojects/caliper/orchestration/caliper_invocation.pyprojects/caliper/orchestration/postprocess.pyprojects/caliper/orchestration/postprocess_logging.py
|
/test fournos skeleton |
🟢 Execution of
|
🟢 Submission of
|
|
closing, will be merged as par of #136 |
1 participant
Summary by CodeRabbit
--format(hierarchicaldefault orjsonl).