Skip to content

fix(rocinsight): resolve 6 pre-existing test failures - #4968

Merged
ammarwa merged 1 commit into
ROCm:developfrom
ammarwa:rocm21553/pr0-fix-preexisting-test-failures
Apr 13, 2026
Merged

fix(rocinsight): resolve 6 pre-existing test failures#4968
ammarwa merged 1 commit into
ROCm:developfrom
ammarwa:rocm21553/pr0-fix-preexisting-test-failures

Conversation

@ammarwa

@ammarwa ammarwa commented Apr 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fix INFO recommendation bucketing: INFO → medium_priority (was incorrectly going to low_priority)
  • Fix WorkflowSession._conv test assertions: tests updated to match code that added conversation persistence via _ensure_conv()
  • Fix WorkflowSession.new() test stubs: add missing _conv=None attribute

Context

These 6 test failures existed on develop before any ROCM-21553 work. Separated into own PR for clean review.

Test plan

  • 565 tests passing, 0 failures (was 559 passed, 6 failed)
  • No behavioral changes to production code except INFO bucketing fix

🤖 Generated with Claude Code

@ammarwa @claude
fix: resolve 6 pre-existing test failures
f62f7e6 
These failures existed before any ROCM-21553 changes. Root causes:

1. INFO recommendation bucketing (api.py):
   _build_analysis_result() put INFO recs into low_priority, but INFO
   recommendations are actionable guidance (e.g. "collect more data")
   and belong in medium_priority. Fixed: INFO now joins MEDIUM bucket.
   Two contradictory tests (one expecting low, one expecting medium)
   aligned to the correct behavior.

2. WorkflowSession._conv attribute (3 test files):
   WorkflowSession gained _conv via _ensure_conv() for conversation
   persistence, but 3 tests still asserted `not hasattr(ws, "_conv")`.
   Fixed: tests now assert `ws._conv is None` (correct — None without
   LLM provider, lazy-initialized when needed).

3. WorkflowSession.__new__() test stubs (test_claude_code_provider.py):
   Tests using __new__() to skip __init__() didn't set _conv=None,
   causing AttributeError when _llm_rewrite_file called _ensure_conv().
   Fixed: added _conv=None to test stubs.

565 tests passing, 0 failures.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 13, 2026
Merged
Merged
Merged
Merged
This was referenced Apr 13, 2026
Merged
Merged
Closed
Merged
@ammarwa
ammarwa requested a review from bwelton April 13, 2026 20:30
@ammarwa
ammarwa marked this pull request as ready for review April 13, 2026 20:30
@ammarwa
ammarwa merged commit ce4f34f into ROCm:develop Apr 13, 2026
20 checks passed
ammarwa added a commit that referenced this pull request Apr 14, 2026
@ammarwa @claude
feat(rocinsight): rename Compute Bottleneck, add confidence %, data-d…
f942b63 
…riven impact, warmup detection (#4969)

## Summary
ROCM-21553 items C3, C4, I3:

- **C3**: Rename "Compute Bottleneck" to context-aware categories
(Kernel Hotspot / Compute-Bound / Memory-Bound / Mixed) with confidence
%
- **C4**: Replace 7 generic estimated_impact strings with data-driven
bounds
- **I3**: New `detect_warmup_issues()` — flags first dispatch >2x
average

## Stacking note
Stacks on #4968. GitHub diff against `develop` includes #4968's changes.
**This PR's own changes (14 files, +426/-35):**
- `analysis/recommendations.py` — category rename, confidence,
data-driven impact
- `analysis/core.py` — `detect_warmup_issues()`
- `analysis/__init__.py`, `analyze.py` — wiring
- `formatters/__init__.py`, `_common.py`, `json_fmt.py`, `markdown.py`,
`webview.py` — confidence display
- `ai_analysis/docs/analysis-output.schema.json` — confidence field
- `test_recommendation_confidence.py` — 14 new tests
- Updated existing tests for renamed categories

Merge after #4968 lands, then rebase for clean diff.

## Test plan
- [x] 14 new tests, 579 total passing, 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ammarwa added a commit that referenced this pull request Apr 14, 2026
@ammarwa @claude
feat(rocinsight): expose private/local LLM providers in CLI, deprecat…
b9bcb35 
…e output_format param (#4981)

## Summary
- **feat**: Added `private` and `local` to `--llm` CLI choices. These
providers were already supported by the LLM layer (private = enterprise
OpenAI-compatible endpoint, local = Ollama) but not exposed in the
argument parser.
- **deprecate**: `output_format` parameter in `analyze_database()` now
emits `DeprecationWarning` when passed (backwards compatible — not
removed). Callers should use `result.to_json()`, `result.to_webview()`,
or `result.to_text()` instead.

## Stacking note
This PR is based on the ROCM-21553 feature stack (#4968 → #4969 → #4970
→ #4971 → #4972). The GitHub diff against `develop` includes those prior
changes. **This PR's own changes are only 3 files:**
- `rocinsight/analyze.py` — 1 line (--llm choices)
- `rocinsight/ai_analysis/api.py` — output_format deprecation
- `rocinsight/ai_analysis/tests/test_cli_llm_providers.py` — 5 new tests

Merge after #4972 lands, then rebase onto develop for a clean diff.

## Test plan
- [x] 5 new tests, all passing
- [x] Backwards compatible — existing callers passing output_format get
a warning, not an error

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ammarwa added a commit that referenced this pull request Apr 17, 2026
@ammarwa @claude
fix(rocinsight): pass ATT data through to_json()/to_webview() for Tie…
3dc7740 
…r 3 serialization (#4978)

## Summary
`to_json()` and `to_webview()` on `AnalysisResult` did not forward
`att_analysis` to `format_analysis_output()`, causing Tier 3 ATT results
to be silently dropped from serialized output despite `analysis_tier=3`.

## Test plan
- [x] 4 new tests verifying att_trace in JSON, schema version 0.4.0, ATT
in webview
- [x] E2E: `analyze_database(att_dir=...)` → `to_json()` → att_trace
present

🤖 Generated with [Claude Code](https://claude.com/claude-code)

## Stacking note
This PR is based on the ROCM-21553 feature stack (#4968-#4972). Its own
change is only: `ai_analysis/api.py` (2 lines adding `att_analysis`
param) + `test_att_serialization.py` (4 tests).

Merge after #4972 lands, then rebase onto develop for a clean diff.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ammarwa added a commit that referenced this pull request Apr 17, 2026
@ammarwa @claude
fix(rocinsight): correct rocprof-sys and rocprof-compute command synt…
bb99293 
…ax (#4972)

## Summary
Verified all recommended commands against installed tools and ROCm docs:

- `rocprof-sys` (deprecated wrapper) → `rocprof-sys-sample`
(recommended)
- `--trace-gpu-memory` (nonexistent) → `--trace --device`
- `--hip-api-trace` stale full_command → `--hip-trace`
- Schema tool enum expanded with all valid binaries
- SCHEMA_CHANGELOG v0.4.0 entry
- Test files renamed from ticket numbers to descriptive names

## Stacking note
Stacks on #4971 → #4970 → #4969 → #4968. GitHub diff includes prior
changes. **This PR's own changes (10 files, +98/-118):**
- `analysis/recommendations.py` — rocprof-sys-sample, --hip-trace
full_command fix
- `ai_analysis/source_analyzer.py` — rocprof-sys-sample commands
- `ai_analysis/docs/analysis-output.schema.json` — tool enum, examples
- `ai_analysis/docs/SCHEMA_CHANGELOG.md` — v0.4.0 entry
- Test file renames (3 files)
- `tests/test_analyze.py`, `tests/test_analyze_schema.py` — tool name
updates

Merge after #4971 lands.

## Test plan
- [x] 594 tests passing, 0 failures
- [x] Verified against `rocprof-compute --help` and `rocprof-sys-sample
--help`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ammarwa added a commit that referenced this pull request Apr 17, 2026
@ammarwa @claude
feat(rocinsight): add roctx region analysis, profiling scope recommen…
f98eafe 
…dations (#4971)

## Summary
ROCM-21553 items C1, E1, E2, 11:

- **C1**: New `analyze_roctx_regions()` — discovers markers, correlates
events by overlap (innermost-wins model)
- **11**: GPU idle warning scoped to figure-of-merit region
- **E1**: `rocprof-compute profile` listed first in Kernel Hotspot
commands
- **E2**: "Profiling Scope" recommendation with
`roctxProfilerPause()`/`roctxProfilerResume()` code snippet
- ROCTx attribution model: innermost-wins with no double-counting,
documented per ROC-TX library spec

## Stacking note
Stacks on #4970 → #4969 → #4968. GitHub diff includes prior changes.
**This PR's own changes (8 files, +540/-38):**
- `analysis/core.py` — `analyze_roctx_regions()` with documented
attribution model
- `analysis/recommendations.py` — roctx_regions param, GPU idle scoping,
E1 command order, E2 profiler pause
- `formatters/__init__.py` — ROCTX RANGE ANALYSIS section
- `formatters/json_fmt.py` — roctx_regions in JSON
- `analyze.py`, `ai_analysis/api.py` — wiring
- `test_roctx_region_analysis.py` — 7 new tests

Merge after #4970 lands.

## Test plan
- [x] 7 new tests, 594 total passing, 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ammarwa added a commit that referenced this pull request Apr 17, 2026
@ammarwa @claude
feat(rocinsight): add per-API overhead breakdown and kernel resource/…
9f488e1 
…occupancy analysis (#4970)

## Summary
ROCM-21553 items C2, I1, I2:

- **C2+I2**: New `analyze_api_overhead()` — per-API call duration
breakdown, separates hipLaunchKernel from total
- **I1**: New `analyze_kernel_resources()` — VGPR/SGPR/LDS/scratch +
theoretical occupancy
- Fix: `--hip-api-trace` (invalid) replaced with `--hip-trace`

## Stacking note
Stacks on #4969 → #4968. GitHub diff includes prior changes. **This PR's
own changes (8 files, +466/-2):**
- `analysis/core.py` — `analyze_api_overhead()`,
`analyze_kernel_resources()`, `_ARCH_SPECS`
- `analysis/recommendations.py` — `api_overhead` param, per-API issue
text
- `formatters/__init__.py` — per-API annotation + KERNEL RESOURCES
section
- `formatters/json_fmt.py` — kernel_resources + api_breakdown in JSON
- `analyze.py`, `ai_analysis/api.py` — wiring
- `test_api_overhead_kernel_resources.py` — 8 new tests

Merge after #4969 lands.

## Test plan
- [x] 8 new tests, 587 total passing, 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ammarwa added a commit that referenced this pull request Apr 18, 2026
@ammarwa @claude
fix(rocinsight): replace --kernel-names with --kernel-include-regex p…
6bf2066 
…er AMD docs (#4980)

## Summary
`--kernel-names` is not in current rocprofv3 documentation. AMD docs
specify `--kernel-include-regex` for kernel filtering. Updated all
recommendations, reference guides, interactive session, and tests.
Kernel names with regex special characters are properly escaped with
`re.escape()`.

## Test plan
- [x] 4 new tests: regex flag used, old flag absent, special chars
escaped, filter arg classification
- [x] 0 occurrences of --kernel-names in output

🤖 Generated with [Claude Code](https://claude.com/claude-code)

## Stacking note
This PR is based on the ROCM-21553 feature stack (#4968-#4972). Its own
change is: `recommendations.py`, `interactive.py`, reference guides, and
tests — all replacing `--kernel-names` with `--kernel-include-regex`.

Merge after #4972 lands, then rebase onto develop for a clean diff.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwelton bwelton bwelton approved these changes

Assignees

No one assigned

Labels

organization: ROCm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ammarwa @bwelton