Add --gpu-cost-per-hour CLI arg to calculate_metrics - #20
Conversation
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe 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. ChangesGPU Cost Configuration
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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/calculate_metrics.py (2)
143-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd 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
/ 3600conversion 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 winMake
num_gpusexplicitly optional — useint | Noneforcompute_metrics_legacy,compute_metrics_latest, andcreate_job_reportinstead ofint = 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
📒 Files selected for processing (1)
scripts/calculate_metrics.py
de032e5 to
d69892f
Compare
Summary
Usage
Changes
GPU_COST_USD_PER_SECOND = 4 / 3600withDEFAULT_GPU_COST_USD_PER_HOUR = 4--gpu-cost-per-hourCLI argumentcompute_metrics_legacy,compute_metrics_latest, andcreate_job_report