Skip to content

Add --gpu-cost-per-hour CLI arg to calculate_metrics - #20

Merged
taagarwa-rh merged 2 commits into
redhat-et:mainfrom
rounakbende10:feat/gpu-cost-cli-arg
Jul 29, 2026
Merged

Add --gpu-cost-per-hour CLI arg to calculate_metrics#20
taagarwa-rh merged 2 commits into
redhat-et:mainfrom
rounakbende10:feat/gpu-cost-cli-arg

Conversation

@rounakbende10

Copy link
Copy Markdown
Contributor

Summary

  • Makes GPU cost per hour a CLI argument instead of hardcoding $4/hr
  • Different GPU providers and types have different rates (e.g. Brev H100 @ $5/hr vs default $4/hr)
  • Default remains $4/hr for backward compatibility

Usage

# Default $4/hr per GPU
python calculate_metrics.py job_dir --num-gpus 4

# Custom rate (e.g. Brev H100 @ $5/hr)
python calculate_metrics.py job_dir --num-gpus 2 --gpu-cost-per-hour 5

Changes

  • Replaced hardcoded GPU_COST_USD_PER_SECOND = 4 / 3600 with DEFAULT_GPU_COST_USD_PER_HOUR = 4
  • Added --gpu-cost-per-hour CLI argument
  • Passed through to compute_metrics_legacy, compute_metrics_latest, and create_job_report
  • Report template snippet now uses the configured rate

GPU cost per hour varies by provider and GPU type. Make it a CLI
argument instead of hardcoding $4/hr. Default remains $4/hr for
backward compatibility.

Usage: python calculate_metrics.py job_dir --num-gpus 2 --gpu-cost-per-hour 5
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 89682186-51d8-4b6b-8987-f2ef1453bf66

📥 Commits

Reviewing files that changed from the base of the PR and between de032e5 and d69892f.

📒 Files selected for processing (1)
  • scripts/calculate_metrics.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • scripts/calculate_metrics.py

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Added a configurable GPU cost-per-hour option for metric calculations and job reports.
    • Introduced the --gpu-cost-per-hour command-line flag (default: $4 per GPU hour).
    • Job reports now reflect the configured GPU pricing.
  • Bug Fixes
    • Improved cost estimates when job results don’t include cost, deriving cost from execution time using the provided hourly rate.
    • Added input validation to reject negative and non-finite GPU cost values.

Walkthrough

The metrics script adds configurable hourly GPU pricing, validates the new CLI value, applies it to fallback cost calculations, and propagates it into generated GPU cost report snippets.

Changes

GPU Cost Configuration

Layer / File(s) Summary
CLI rate and metric fallback calculation
scripts/calculate_metrics.py
Adds a validated --gpu-cost-per-hour option, changes the default cost constant to an hourly value, and applies the configured rate to legacy and latest fallback calculations.
Report rate propagation and rendering
scripts/calculate_metrics.py
Passes the configured rate through main() and uses it in generated GPU cost snippets.

Estimated code review effort: 3 (Moderate) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant main
  participant compute_metrics_legacy
  participant compute_metrics_latest
  participant create_job_report
  CLI->>main: Parse gpu_cost_per_hour
  main->>compute_metrics_legacy: Pass hourly GPU rate
  main->>compute_metrics_latest: Pass hourly GPU rate
  compute_metrics_legacy-->>main: Return fallback cost
  compute_metrics_latest-->>main: Return fallback cost
  main->>create_job_report: Pass hourly GPU rate
  create_job_report-->>main: Render GPU cost snippet
Loading

Suggested reviewers: hannahsmith-rh

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: adding a configurable GPU cost-per-hour CLI argument to calculate_metrics.
Description check ✅ Passed The description is directly related and accurately summarizes the GPU cost-per-hour CLI change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
scripts/calculate_metrics.py (2)

143-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for both job formats.

Test a non-default hourly rate with fixed GPU count, concurrency, and agent time in both legacy and latest paths, asserting the / 3600 conversion and resulting cost.

Also applies to: 211-211

🤖 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 `@scripts/calculate_metrics.py` at line 143, Add regression tests covering both
the legacy and latest job-format paths that calculate cost, using a non-default
hourly rate with fixed GPU count, concurrency, and agent time. Assert that the
calculation includes the /3600 hourly-to-second conversion and produces the
expected rounded cost, targeting the cost calculation used by each path.

69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make num_gpus explicitly optional — use int | None for compute_metrics_legacy, compute_metrics_latest, and create_job_report instead of int = None.

🤖 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 `@scripts/calculate_metrics.py` at line 69, Update the type annotations for
num_gpus in compute_metrics_legacy, compute_metrics_latest, and
create_job_report to use int | None while preserving their existing default
value and behavior.

Source: Linters/SAST tools

🤖 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 `@scripts/calculate_metrics.py`:
- Around line 55-56: Update the argparse definition for --gpu-cost-per-hour to
validate each supplied rate as finite and non-negative before returning it,
rejecting negative, NaN, and infinite values at the CLI boundary. Preserve
DEFAULT_GPU_COST_USD_PER_HOUR as the default and ensure invalid input produces
the parser’s standard argument error.

---

Nitpick comments:
In `@scripts/calculate_metrics.py`:
- Line 143: Add regression tests covering both the legacy and latest job-format
paths that calculate cost, using a non-default hourly rate with fixed GPU count,
concurrency, and agent time. Assert that the calculation includes the /3600
hourly-to-second conversion and produces the expected rounded cost, targeting
the cost calculation used by each path.
- Line 69: Update the type annotations for num_gpus in compute_metrics_legacy,
compute_metrics_latest, and create_job_report to use int | None while preserving
their existing default value and behavior.
🪄 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: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: fad6fbf3-4f95-4866-92fc-de27b51cf5f5

📥 Commits

Reviewing files that changed from the base of the PR and between a3d9501 and 11a8594.

📒 Files selected for processing (1)
  • scripts/calculate_metrics.py

Comment thread scripts/calculate_metrics.py Outdated
@hannahsmith-rh
hannahsmith-rh self-requested a review July 23, 2026 16:52
@rounakbende10
rounakbende10 force-pushed the feat/gpu-cost-cli-arg branch from de032e5 to d69892f Compare July 23, 2026 16:58
@taagarwa-rh
taagarwa-rh merged commit fe98c05 into redhat-et:main Jul 29, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants