feat(application): Add option to summarize run describe.#414
feat(application): Add option to summarize run describe.#414blanca-pablos wants to merge 3 commits intomainfrom
Conversation
3e986b0 to
394b89b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
|
src/aignostics/application/_utils.py
Outdated
| if summarize: | ||
| _print_run_summary(run, run_handle) | ||
| else: | ||
| run_details = _format_run_details(run) | ||
| output = f"[bold]Run Details for {run.run_id}[/bold]\n{'=' * 80}\n{run_details}\n\n[bold]Items:[/bold]" | ||
| console.print(output) | ||
| _retrieve_and_print_run_items(run_handle) |
There was a problem hiding this comment.
It seems like _print_run_summary has some overlap with _format_run_details. I would suggest something like this:
| if summarize: | |
| _print_run_summary(run, run_handle) | |
| else: | |
| run_details = _format_run_details(run) | |
| output = f"[bold]Run Details for {run.run_id}[/bold]\n{'=' * 80}\n{run_details}\n\n[bold]Items:[/bold]" | |
| console.print(output) | |
| _retrieve_and_print_run_items(run_handle) | |
| run_details = _format_run_details(run) | |
| output = f"[bold]Run Details for {run.run_id}[/bold]\n{'=' * 80}\n{run_details}\n\n[bold]Items:[/bold]" | |
| console.print(output) | |
| _retrieve_and_print_run_items(run_handle, summarize) |
And then extending _retrieve_and_print_run_items to handle the summary option (essentially skipping the output artifacts part AFAICT).
There was a problem hiding this comment.
yep you're right, addressed!
Co-authored-by: Oliver Meyer <42039965+olivermeyer@users.noreply.github.com>
| str: Formatted status string | ||
| """ | ||
| if state is RunState.TERMINATED and termination_reason: | ||
| if (state.value == RunState.TERMINATED or state.value == ItemState.TERMINATED) and termination_reason: |
There was a problem hiding this comment.
Bug: The comparison state.value == RunState.TERMINATED incorrectly compares a string to an enum object, which will always evaluate to false, hiding the termination reason.
Severity: MEDIUM
Suggested Fix
Change the condition to compare enum members directly, for example: if (state is RunState.TERMINATED or state is ItemState.TERMINATED) and termination_reason:. Alternatively, compare the string values consistently: if state.value == 'TERMINATED' and termination_reason:.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/aignostics/application/_utils.py#L188
Potential issue: In the `_format_status_string` function, the condition `state.value ==
RunState.TERMINATED` incorrectly compares a string value (e.g., `'TERMINATED'`) with an
enum member object (`RunState.TERMINATED`). Because `RunState` and `ItemState` inherit
from `(str, Enum)` and not the more modern `StrEnum`, this comparison will always
evaluate to `False`. As a result, the termination reason is never appended to the status
string for terminated runs or items. This prevents users from seeing why a process was
terminated, degrading the utility of status summaries.
Did we get this right? 👍 / 👎 to inform future reviews.
PR #414 Review: feat(application): Add option to summarize run describeExecutive SummaryStatus: ❌ BLOCKING ISSUES FOUND This PR adds a ✅ Strengths
🚫 BLOCKING ISSUES (Must Fix)1. Ruff Linting Failure (CRITICAL)Location: Issue: Complexity check violation - compound condition should use set membership # CURRENT (line 188):
if (state.value == RunState.TERMINATED or state.value == ItemState.TERMINATED) and termination_reason:
return f"{state.value} ({termination_reason})"Linting Error: Fix: Refactor to use set membership for cleaner comparison: # OPTION 1 (Recommended - Most Pythonic):
if state.value in {RunState.TERMINATED.value, ItemState.TERMINATED.value} and termination_reason:
return f"{state.value} ({termination_reason})"
# OPTION 2 (Alternative - Direct enum comparison):
if state in {RunState.TERMINATED, ItemState.TERMINATED} and termination_reason:
return f"{state.value} ({termination_reason})"Why This Matters: This is medical device software. Linting rules enforce code quality standards that prevent bugs.
Verify Fix: make lint💡 SUGGESTIONS (Non-Blocking)1. Help Text ImprovementLocation: # CURRENT:
help="Show only run and item status summary (external ID, state, error message)"
# SUGGESTION (more explicit about what's hidden):
help="Show concise status summary (omits artifact download URLs and metadata)"This makes it clearer what the flag does (omits verbose output) rather than what it shows. ✅ Architecture Compliance
🎯 Testing VerificationTest Markers Check# Verified: All tests have required markers
uv run pytest tests/aignostics/application/utils_test.py -m "not unit and not integration and not e2e" --collect-only
# Result: 61 collected / 61 deselected / 0 selected ✅Coverage Impact
📋 Required Actions Before MergeMUST FIX (Blocking)
RECOMMENDED (Non-Blocking)
🔍 Final Verification CommandsAfter fixing the linting issue, run: # Full validation pipeline
make lint && make test_unit
# Or the complete suite
make allExpected result: All checks pass ✅ 🎖️ Excellent Work On
Once the linting issue is fixed, this PR will be ready to merge. 🚀 Reviewed by: Claude Code (AI-powered code review) |
|


To facilitate overview of run progress for users, specially for runs with large numbers of items, add a
--sumarizeoption toapplication run describethat just outputs run status and item status + error message, if given.