Skip to content

Conversation

@amaslenn
Copy link
Contributor

Summary

Order should be: [per_test, ..., status, tarball].

Test Plan

  1. CI (extended)
  2. Manual runs

Additional Notes

Order should be: [per_test, ..., status, tarball]
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 16, 2025

📝 Walkthrough

Walkthrough

Introduces a new public method Registry.ordered_scenario_reports() that returns scenario reports ordered by priority: "per_test" first, "status" second-to-last, and "tarball" last. The CLI handler is updated to use this method instead of inline sorting logic, with a corresponding test added to verify the ordering.

Changes

Cohort / File(s) Summary
Registry enhancement
src/cloudai/_core/registry.py
Adds ordered_scenario_reports() public method that returns a list of (name, Reporter) tuples sorted by internal priority mapping (per_test=0, unknown=1, status=2, tarball=3)
Handler integration
src/cloudai/cli/handlers.py
Replaces inline scenario_reports sorting logic with call to registry.ordered_scenario_reports(); removes explicit comment about status report ordering
Test coverage
tests/test_reporter.py
Adds test_report_order() function to verify scenario reports are ordered correctly: first key is "per_test", penultimate is "status", last is "tarball"

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Straightforward feature extraction: moving inline sorting logic into a dedicated, well-defined method with clear priority rules
  • All changes are tightly coupled and self-contained—no ripple effects across other modules
  • Test coverage is minimal but sufficient to validate the ordering guarantee

Poem

🐰 The reports hop in perfect line,
Per-test first—the grand design!
Status close to journey's end,
Tarball waits round the final bend,
Order emerges, clean and fine! 🎩✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Ensure reports order' directly aligns with the main change: introducing an ordered_scenario_reports method to enforce a specific report ordering (per_test first, status second-to-last, tarball last).
Description check ✅ Passed The description specifies the desired report order and includes a test plan, clearly relating to the changeset which implements this ordering across registry, handlers, and tests.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch am/report-order

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.

❤️ Share

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

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 16, 2025

Greptile Overview

Greptile Summary

Refactored report generation to enforce deterministic ordering: per_test report first, status second-to-last, and tarball last, with other reports in the middle.

Key changes:

  • Added Registry.ordered_scenario_reports() method that centralizes report ordering logic using explicit priority values (0=first, 1=middle, 2=second-to-last, 3=last)
  • Replaced ad-hoc sorting in handlers.py with call to the new method for cleaner code
  • Added unit test to verify the ordering is maintained correctly

The implementation is clean and well-tested. The ordering logic correctly handles the 5 currently registered reports: per_test, nixl_bench_summary, nccl_comparison, status, and tarball.

Confidence Score: 5/5

  • This PR is safe to merge with no identified issues
  • The changes are simple, well-contained, and properly tested. The new method centralizes report ordering logic, making it more maintainable. The test coverage validates the expected behavior, and the logic correctly handles all currently registered reports.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/cloudai/_core/registry.py 5/5 Added ordered_scenario_reports() method to enforce report ordering: per_test (first), other reports (middle), status (second-to-last), tarball (last)
src/cloudai/cli/handlers.py 5/5 Replaced custom sorting logic with call to registry.ordered_scenario_reports() for cleaner, centralized report ordering
tests/test_reporter.py 5/5 Added test to verify report ordering: per_test first, status second-to-last, tarball last

Sequence Diagram

sequenceDiagram
    participant H as handlers.py
    participant R as Registry
    participant Rep as Reporter Classes
    
    H->>R: ordered_scenario_reports()
    activate R
    R->>R: report_order(k) for each report
    Note over R: per_test=0, status=2,<br/>tarball=3, others=1
    R->>R: sorted(scenario_reports.items())
    R-->>H: [(name, reporter_class), ...]
    deactivate R
    
    loop For each report in order
        H->>H: Get report config
        alt Report enabled
            H->>Rep: Instantiate reporter_class
            H->>Rep: Generate report
        else Report disabled
            H->>H: Skip report
        end
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82662c9 and 3fc2c49.

📒 Files selected for processing (3)
  • src/cloudai/_core/registry.py (1 hunks)
  • src/cloudai/cli/handlers.py (1 hunks)
  • tests/test_reporter.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/cloudai/_core/registry.py (1)
src/cloudai/_core/base_reporter.py (1)
  • Reporter (44-85)
tests/test_reporter.py (1)
src/cloudai/_core/registry.py (1)
  • ordered_scenario_reports (226-234)
src/cloudai/cli/handlers.py (1)
src/cloudai/_core/registry.py (1)
  • ordered_scenario_reports (226-234)
🔇 Additional comments (1)
src/cloudai/cli/handlers.py (1)

172-172: LGTM! Clean refactoring that centralizes ordering logic.

Replacing the inline iteration with a call to registry.ordered_scenario_reports() improves maintainability by centralizing the report ordering logic in the Registry class, eliminating the need for inline comments about ordering requirements.

@amaslenn amaslenn merged commit ab05b2b into main Dec 16, 2025
5 checks passed
@amaslenn amaslenn deleted the am/report-order branch December 16, 2025 20:04
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.

3 participants