[caliper] Progress with the parser and plotter#57
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplits Skeleton postprocess into parser/KPI/plot modules; adds GuideLLM parser/models/KPI/plugin; introduces HelpfulGroup and adopts it across orchestration CLIs; simplifies Caliper CLI options and adds an ChangesSkeleton Plugin Component Separation
GuideLLM Postprocess Plugin
CLI Infrastructure: Error Handling and Caliper CLI Updates
Replot Helper and FOURNOS Directive
Fournos Launcher: Job-name Validation & Sanitization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
|
/test fournos llm_d |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 20 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 19 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
projects/skeleton/postprocess/default/plugin.py (1)
54-58: ⚡ Quick winSilently skipping unknown report IDs may hide configuration errors.
When a requested
report_iddoesn't exist inself.plots, it's silently ignored. Users might not realize why an expected report wasn't generated, especially if the ID contains a typo.📝 Proposed improvement to warn about unknown IDs
for report_id in wanted: if report_id in self.plots: plot_class = self.plots[report_id] path = plot_class.generate(model, output_dir) paths.append(path) + else: + # Log or collect warning about unknown report_id + passAlternatively, consider validating report IDs upfront or documenting the silent-skip behavior.
🤖 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/skeleton/postprocess/default/plugin.py` around lines 54 - 58, The loop that iterates over wanted and silently skips report IDs not in self.plots should surface a warning or validation: in the block where report_id is checked against self.plots (the for report_id in wanted loop that calls plot_class.generate), add logic to collect or log unknown IDs (e.g., using the module/class logger or raising a clear ValueError) so users are notified when a requested report_id is missing; you can either validate all wanted IDs up-front against self.plots (and raise/log all unknown IDs) or log a warning for each missing report_id before continuing to call plot_class.generate for known IDs.projects/skeleton/postprocess/default/plotting/summary_table.py (1)
28-30: ⚡ Quick winConsider using "N/A" for missing metric values.
Empty string defaults for missing
scenario,throughput, andlatency_msvalues will render as blank table cells, which may be unclear to users. Consider using"N/A"or similar placeholder text to explicitly indicate missing data.📋 Proposed improvement
for r in model.unified_result_records: - scenario = r.distinguishing_labels.get("scenario", "") - tp = r.metrics.get("throughput", "") - lat = r.metrics.get("latency_ms", "") + scenario = r.distinguishing_labels.get("scenario", "N/A") + tp = r.metrics.get("throughput", "N/A") + lat = r.metrics.get("latency_ms", "N/A")🤖 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/skeleton/postprocess/default/plotting/summary_table.py` around lines 28 - 30, Change the empty-string defaults for missing labels/metrics to a clear placeholder so table cells show "N/A" instead of blank; update the lines that set scenario, tp, and lat (currently using r.distinguishing_labels.get("scenario", "") and r.metrics.get("throughput", "") / r.metrics.get("latency_ms", "")) to use "N/A" as the default value; ensure any downstream code that formats or displays these variables (e.g., the summary table rendering for scenario, tp, lat) continues to treat them as strings.
🤖 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/skeleton/postprocess/default/parsing/kpis.py`:
- Around line 56-65: The float conversions for tp_raw and lat_raw currently
swallow errors and set values to 0.0, hiding bad input; update the try/except
blocks that set tp and lat (variables tp_raw, lat_raw -> tp, lat in
projects/skeleton/postprocess/default/parsing/kpis.py) to catch exceptions but
log a warning/error including the offending raw value and exception details (use
the module's logger or process logger), then assign a sentinel like None (or
math.nan) instead of 0.0 so downstream code can detect malformed data and handle
it explicitly; also adjust any downstream consumers to tolerate None/NaN if
needed.
In `@projects/skeleton/postprocess/default/parsing/parsers.py`:
- Around line 16-24: In _labels_from_node, avoid calling raw.get("labels") on
node.labels when node.labels may not be a dict: first check if raw (node.labels)
is a dict, then attempt inner = raw.get("labels"); if inner is a dict return a
shallow copy, if not but raw is a dict return a shallow copy of raw; for all
other non-dict raw values (None, str, etc.) return the default {"facet":
"default"}; update the logic in _labels_from_node to use isinstance(raw, dict)
before any .get calls to prevent AttributeError.
- Around line 45-47: In the except json.JSONDecodeError block inside the JSON
parsing routine (the except json.JSONDecodeError as e where warnings.append(...)
is called), stop returning the sentinel dict {"_parse_error": True} and instead
return an empty dict so downstream logic like the metrics fallback check (the
`if metrics else {"throughput": 0.0}` branch) will kick in; ensure the function
still returns ({}, warnings) so UnifiedResultRecord and other consumers receive
an empty metrics dict rather than a truthy error marker.
In `@projects/skeleton/postprocess/default/plotting/throughput_chart.py`:
- Around line 34-37: The try/except that coerces raw to float (the block
converting variable raw into y) currently swallows TypeError/ValueError and sets
y=0.0; change it to log a warning when conversion fails, e.g. call
logger.warning or logging.warning with the offending raw value and any
identifying context (timestamp/index/metric name if available) before assigning
y=0.0 so malformed inputs are visible; locate the conversion of raw -> y in
throughput_chart.py (the try/except handling TypeError/ValueError) and add the
warning call using the module logger (or import logging and create one if
absent).
---
Nitpick comments:
In `@projects/skeleton/postprocess/default/plotting/summary_table.py`:
- Around line 28-30: Change the empty-string defaults for missing labels/metrics
to a clear placeholder so table cells show "N/A" instead of blank; update the
lines that set scenario, tp, and lat (currently using
r.distinguishing_labels.get("scenario", "") and r.metrics.get("throughput", "")
/ r.metrics.get("latency_ms", "")) to use "N/A" as the default value; ensure any
downstream code that formats or displays these variables (e.g., the summary
table rendering for scenario, tp, lat) continues to treat them as strings.
In `@projects/skeleton/postprocess/default/plugin.py`:
- Around line 54-58: The loop that iterates over wanted and silently skips
report IDs not in self.plots should surface a warning or validation: in the
block where report_id is checked against self.plots (the for report_id in wanted
loop that calls plot_class.generate), add logic to collect or log unknown IDs
(e.g., using the module/class logger or raising a clear ValueError) so users are
notified when a requested report_id is missing; you can either validate all
wanted IDs up-front against self.plots (and raise/log all unknown IDs) or log a
warning for each missing report_id before continuing to call plot_class.generate
for known IDs.
🪄 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: e6e1d1d4-bb8a-4f9b-a3f9-23ed64ac918b
📒 Files selected for processing (15)
projects/caliper/cli/main.pyprojects/core/library/ci.pyprojects/foreign_testing/orchestration/ci.pyprojects/fournos_launcher/orchestration/ci.pyprojects/jump_ci/orchestration/ci.pyprojects/llm_d/orchestration/ci.pyprojects/llm_d_legacy/orchestration/ci.pyprojects/skeleton/orchestration/ci.pyprojects/skeleton/postprocess/default/parsing/__init__.pyprojects/skeleton/postprocess/default/parsing/kpis.pyprojects/skeleton/postprocess/default/parsing/parsers.pyprojects/skeleton/postprocess/default/plotting/__init__.pyprojects/skeleton/postprocess/default/plotting/summary_table.pyprojects/skeleton/postprocess/default/plotting/throughput_chart.pyprojects/skeleton/postprocess/default/plugin.py
| try: | ||
| tp = float(tp_raw) | ||
| except (TypeError, ValueError): | ||
| tp = 0.0 | ||
|
|
||
| # Convert latency to float | ||
| try: | ||
| lat = float(lat_raw) | ||
| except (TypeError, ValueError): | ||
| lat = 0.0 |
There was a problem hiding this comment.
Silent conversion failures mask data quality issues.
Similar to throughput_chart.py (lines 34-37), failed float conversions default silently to 0.0. This pattern appears in multiple places and may hide malformed input data.
🔍 Proposed improvement
# Convert throughput to float
try:
tp = float(tp_raw)
except (TypeError, ValueError):
+ # Could log: f"Invalid throughput {tp_raw!r} in {r.test_base_path}"
tp = 0.0
# Convert latency to float
try:
lat = float(lat_raw)
except (TypeError, ValueError):
+ # Could log: f"Invalid latency {lat_raw!r} in {r.test_base_path}"
lat = 0.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| tp = float(tp_raw) | |
| except (TypeError, ValueError): | |
| tp = 0.0 | |
| # Convert latency to float | |
| try: | |
| lat = float(lat_raw) | |
| except (TypeError, ValueError): | |
| lat = 0.0 | |
| try: | |
| tp = float(tp_raw) | |
| except (TypeError, ValueError): | |
| # Could log: f"Invalid throughput {tp_raw!r} in {r.test_base_path}" | |
| tp = 0.0 | |
| # Convert latency to float | |
| try: | |
| lat = float(lat_raw) | |
| except (TypeError, ValueError): | |
| # Could log: f"Invalid latency {lat_raw!r} in {r.test_base_path}" | |
| lat = 0.0 |
🤖 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/skeleton/postprocess/default/parsing/kpis.py` around lines 56 - 65,
The float conversions for tp_raw and lat_raw currently swallow errors and set
values to 0.0, hiding bad input; update the try/except blocks that set tp and
lat (variables tp_raw, lat_raw -> tp, lat in
projects/skeleton/postprocess/default/parsing/kpis.py) to catch exceptions but
log a warning/error including the offending raw value and exception details (use
the module's logger or process logger), then assign a sentinel like None (or
math.nan) instead of 0.0 so downstream code can detect malformed data and handle
it explicitly; also adjust any downstream consumers to tolerate None/NaN if
needed.
| def _labels_from_node(node: TestBaseNode) -> dict[str, Any]: | ||
| """Extract labels from a test node.""" | ||
| raw = node.labels | ||
| inner = raw.get("labels") | ||
| if isinstance(inner, dict): | ||
| return dict(inner) | ||
| if isinstance(raw, dict): | ||
| return dict(raw) | ||
| return {"facet": "default"} |
There was a problem hiding this comment.
AttributeError if node.labels is not a dict.
Line 19 calls raw.get("labels") without first checking if raw (which is node.labels) is a dictionary. If node.labels is None, a string, or any other non-dict type, this will raise an AttributeError.
🛡️ Proposed fix
def _labels_from_node(node: TestBaseNode) -> dict[str, Any]:
"""Extract labels from a test node."""
raw = node.labels
+ if not isinstance(raw, dict):
+ return {"facet": "default"}
inner = raw.get("labels")
if isinstance(inner, dict):
return dict(inner)
- if isinstance(raw, dict):
- return dict(raw)
- return {"facet": "default"}
+ return dict(raw)🤖 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/skeleton/postprocess/default/parsing/parsers.py` around lines 16 -
24, In _labels_from_node, avoid calling raw.get("labels") on node.labels when
node.labels may not be a dict: first check if raw (node.labels) is a dict, then
attempt inner = raw.get("labels"); if inner is a dict return a shallow copy, if
not but raw is a dict return a shallow copy of raw; for all other non-dict raw
values (None, str, etc.) return the default {"facet": "default"}; update the
logic in _labels_from_node to use isinstance(raw, dict) before any .get calls to
prevent AttributeError.
| try: | ||
| y = float(raw) | ||
| except (TypeError, ValueError): | ||
| y = 0.0 |
There was a problem hiding this comment.
Consider logging conversion failures.
Failed float conversions silently default to 0.0, which may mask data quality issues. Users won't be alerted when throughput values are malformed or invalid.
📊 Proposed improvement to add warning
try:
y = float(raw)
except (TypeError, ValueError):
+ # Could log or collect warning: f"Invalid throughput value {raw!r} in {label}"
y = 0.0🤖 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/skeleton/postprocess/default/plotting/throughput_chart.py` around
lines 34 - 37, The try/except that coerces raw to float (the block converting
variable raw into y) currently swallows TypeError/ValueError and sets y=0.0;
change it to log a warning when conversion fails, e.g. call logger.warning or
logging.warning with the offending raw value and any identifying context
(timestamp/index/metric name if available) before assigning y=0.0 so malformed
inputs are visible; locate the conversion of raw -> y in throughput_chart.py
(the try/except handling TypeError/ValueError) and add the warning call using
the module logger (or import logging and create one if absent).
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 11 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 06 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/core/library/replot.py`:
- Around line 24-29: The function run_replot currently ignores the passed
artifact_directory because subsequent path computations use env.ARTIFACT_DIR
directly; update all usages in run_replot (and any helper calls it invokes) to
reference the local parameter artifact_directory (falling back to
env.ARTIFACT_DIR only when the parameter is None) instead of directly using
env.ARTIFACT_DIR — search for symbols artifact_directory and env.ARTIFACT_DIR in
run_replot and replace hard-coded env.ARTIFACT_DIR references (including where
download/output paths are built) so the --artifact-directory argument actually
overrides those paths.
- Around line 95-99: Add a timeout to the subprocess.run that executes the
caliper import command: pass a sensible timeout (e.g., timeout=120) to
subprocess.run(cmd, timeout=..., capture_output=True, text=True, check=False)
and wrap the call in a try/except for subprocess.TimeoutExpired to raise a clear
RuntimeError (e.g., "Caliper import timed out") so a hung child cannot stall CI;
keep using the existing cmd variable and existing error-handling for non-zero
return codes (result.returncode) unchanged.
- Around line 64-93: The initial conditional incorrectly groups "s3://" with
MLflow URLs causing s3 URLs to hit the unsupported branch; update the logic in
replot.py so replot_url.startswith("s3://") is not combined with the MLflow
check: either remove "s3://" from the outer condition and only check "mlflow" in
replot_url.lower() (so the MLflow run-id parsing block using
re.search(r"/runs/([^/]+)", ...) and the cmd construction runs for MLflow URLs),
or add a distinct branch for s3 handling before/after the MLflow branch that
constructs the appropriate caliper import command for S3 instead of raising the
ValueError. Ensure the ValueError raising lines (the else that raises
"Unsupported replot URL type") only run for genuinely unsupported URLs.
In `@projects/fournos_launcher/orchestration/pr_args.py`:
- Around line 207-212: The current parsing uses line.removeprefix("/replot.url
") so a bare "/replot.url" (no trailing space/value) is treated as a non-empty
literal and bypasses the empty check; update the extraction to remove the
"/replot.url" prefix regardless of a following space and then strip whitespace
before validating. Concretely, replace the removeprefix("/replot.url ") usage
with logic that detects and strips "/replot.url" (e.g., remove prefix
"/replot.url" then call .lstrip()/.strip()), assign to replot_url, and keep the
existing ValueError check so an empty or whitespace-only value raises; reference
the replot_url variable and the current removeprefix call in pr_args.py.
🪄 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: 7bee74c8-96d6-453f-be9b-2e9fcd774eea
📒 Files selected for processing (12)
projects/caliper/cli/main.pyprojects/caliper/postprocess/__init__.pyprojects/caliper/postprocess/guidellm/__init__.pyprojects/caliper/postprocess/guidellm/parsing/__init__.pyprojects/caliper/postprocess/guidellm/parsing/kpis.pyprojects/caliper/postprocess/guidellm/parsing/models.pyprojects/caliper/postprocess/guidellm/parsing/parsers.pyprojects/caliper/postprocess/guidellm/plotting/__init__.pyprojects/caliper/postprocess/guidellm/plugin.pyprojects/core/library/replot.pyprojects/fournos_launcher/orchestration/pr_args.pyprojects/llm_d_legacy/orchestration/ci.py
✅ Files skipped from review due to trivial changes (1)
- projects/caliper/postprocess/guidellm/parsing/init.py
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 21 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 21 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 02 minutes 11 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 20 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cli/main.py`:
- Around line 609-615: The exception handling for click.ClickException currently
prints only the help text when exc.ctx exists and drops the actual error
message; update the handler so it calls exc.show(sys.stderr) first to print the
exception message (e.g., from MissingParameter or NoArgsIsHelpError) and then,
if exc.ctx is present, call click.echo(exc.ctx.get_help(), err=True) to also
display the help page; keep the existing sys.exit(2) behavior.
- Around line 527-598: Add the same MLflow auth/TLS options and setup used by
the artifacts export flow to the import_command: add CLI options
--mlflow-secrets, --mlflow-config, and --mlflow-insecure-tls (in addition to
mlflow_tracking_uri) and, before calling mlflow.set_tracking_uri / MlflowClient
in import_command, call the shared MLflow setup function used by export (reuse
the existing export-side initializer — e.g., configure_mlflow_client or
create_mlflow_client if present) to apply credentials, TLS overrides, and config
from the provided flags; ensure the new flags are parsed into the shared setup
and only fall back to mlflow.set_tracking_uri/MlflowClient after that.
🪄 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: e2a9b411-8177-40e3-98d4-c26e39e6a619
📒 Files selected for processing (15)
projects/caliper/cli/main.pyprojects/caliper/engine/file_export/runner.pyprojects/caliper/postprocess/__init__.pyprojects/caliper/postprocess/guidellm/__init__.pyprojects/caliper/postprocess/guidellm/parsing/__init__.pyprojects/caliper/postprocess/guidellm/parsing/kpis.pyprojects/caliper/postprocess/guidellm/parsing/models.pyprojects/caliper/postprocess/guidellm/parsing/parsers.pyprojects/caliper/postprocess/guidellm/plotting/__init__.pyprojects/caliper/postprocess/guidellm/plugin.pyprojects/core/library/replot.pyprojects/fournos_launcher/orchestration/pr_args.pyprojects/llm_d_legacy/orchestration/ci.pyprojects/llm_d_legacy/testing/config.yamlprojects/skeleton/orchestration/config.yaml
✅ Files skipped from review due to trivial changes (2)
- projects/caliper/postprocess/guidellm/parsing/init.py
- projects/caliper/engine/file_export/runner.py
🚧 Files skipped from review as they are similar to previous changes (6)
- projects/caliper/postprocess/guidellm/plugin.py
- projects/caliper/postprocess/guidellm/parsing/kpis.py
- projects/fournos_launcher/orchestration/pr_args.py
- projects/llm_d_legacy/orchestration/ci.py
- projects/caliper/postprocess/guidellm/parsing/parsers.py
- projects/core/library/replot.py
| except click.ClickException as exc: | ||
| # Handle click exceptions including NoArgsIsHelpError and MissingParameter | ||
| if hasattr(exc, "ctx") and exc.ctx: | ||
| click.echo(exc.ctx.get_help(), err=True) | ||
| else: | ||
| exc.show(sys.stderr) | ||
| sys.exit(2) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Click 8.1.0, does click.Context.get_help()include the original exception text forMissingParameterandNoSuchOption, or is exc.show(...) required to preserve it?
💡 Result:
In Click 8.1.0, click.Context.get_help does NOT include the original exception text for MissingParameter or NoSuchOption. In other words, exc.show(...) is required if you want the exception’s message (e.g., the “Missing parameter …” or “No such option …” text) to be preserved and displayed. Reasoning grounded in the code: - Context.get_help just returns the formatted help page generated from the command (it calls command.get_help/format_help) and has no access to any active exception or its message. [1] - ClickException.show is the method responsible for rendering the exception message to stderr/file (including adding the “Try '… --help' for help” hint and then echoing “Error: {message}”). [2] - MissingParameter and NoSuchOption both implement their own message formatting via ClickException.format_message / str logic (so the message lives on the exception object, not on the help text). [2] So: if you call ctx.get_help (or command.get_help(ctx)) in an exception handler without also showing the exception, you will get only usage/help text, not the original exception message; to include it you must call exc.show(...).
Citations:
- 1: https://github.com/pallets/click/blob/master/src/click/core.py
- 2: https://github.com/pallets/click/blob/master/src/click/exceptions.py
Preserve error messages when showing help for Click exceptions.
When exc.ctx is present, the current code shows only the help text and loses the actual error message (e.g., "Missing parameter …" or "No such option …"). Click's Context.get_help() returns only the formatted help page; exc.show() is required to display the exception message. Call exc.show(sys.stderr) first, then optionally show help:
Proposed fix
except click.ClickException as exc:
- # Handle click exceptions including NoArgsIsHelpError and MissingParameter
- if hasattr(exc, "ctx") and exc.ctx:
- click.echo(exc.ctx.get_help(), err=True)
- else:
- exc.show(sys.stderr)
+ # Preserve Click's error text, then add command help when available.
+ exc.show(sys.stderr)
+ if getattr(exc, "ctx", None):
+ click.echo("", err=True)
+ click.echo(exc.ctx.get_help(), err=True)
sys.exit(2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except click.ClickException as exc: | |
| # Handle click exceptions including NoArgsIsHelpError and MissingParameter | |
| if hasattr(exc, "ctx") and exc.ctx: | |
| click.echo(exc.ctx.get_help(), err=True) | |
| else: | |
| exc.show(sys.stderr) | |
| sys.exit(2) | |
| except click.ClickException as exc: | |
| # Preserve Click's error text, then add command help when available. | |
| exc.show(sys.stderr) | |
| if getattr(exc, "ctx", None): | |
| click.echo("", err=True) | |
| click.echo(exc.ctx.get_help(), err=True) | |
| sys.exit(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/cli/main.py` around lines 609 - 615, The exception handling
for click.ClickException currently prints only the help text when exc.ctx exists
and drops the actual error message; update the handler so it calls
exc.show(sys.stderr) first to print the exception message (e.g., from
MissingParameter or NoArgsIsHelpError) and then, if exc.ctx is present, call
click.echo(exc.ctx.get_help(), err=True) to also display the help page; keep the
existing sys.exit(2) behavior.
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 22 minutes 27 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'llm_d_legacy replot' failed after 00 hours 00 minutes 03 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 01 minutes 29 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
kpouget
force-pushed
the
caliper-skeleton
branch
2 times, most recently
from
44df45a to
086b37e
Compare
|
/test fournos llm_d_legacy |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 00 minutes 21 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'llm_d_legacy replot' failed after 00 hours 00 minutes 03 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🔴 Test of 'llm_d_legacy replot' failed after 00 hours 00 minutes 03 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 01 minutes 28 seconds 🟢 • Link to the test results. • No reports generated... Test configuration: |
|
/test fournos llm_d_legacy |
|
🟢 Test of 'fournos_launcher submit' succeeded after 00 hours 01 minutes 29 seconds 🟢 • Link to the test results. • No reports generated... Test configuration: |
|
/test fournos llm_d_legacy |
|
🔴 Test of 'llm_d_legacy test' failed after 00 hours 00 minutes 01 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: |
|
🔴 Test of 'fournos_launcher submit' failed after 00 hours 01 minutes 18 seconds 🔴 • Link to the test results. • No reports generated... Test configuration: • Failure indicator: Empty. |
|
@kpouget: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit
New Features
Improvements
Configuration