Skip to content

Hnimrama/rundeck reports - #288

Open
hnimra-amd wants to merge 15 commits into
mainfrom
hnimrama/rundeck-reports
Open

Hnimrama/rundeck reports#288
hnimra-amd wants to merge 15 commits into
mainfrom
hnimrama/rundeck-reports

Conversation

@hnimra-amd
hnimra-amd marked this pull request as draft August 4, 2026 19:56
@hnimra-amd
hnimra-amd force-pushed the hnimrama/rundeck-reports branch from d8c65af to 8637c59 Compare August 4, 2026 19:56
@hnimra-amd
hnimra-amd marked this pull request as ready for review August 4, 2026 20:20
@solaiys

solaiys commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Review: blocking issues

The Run Deck framework itself is mostly additive and self-contained (the module-global _SESSION store in registry.py is only mutated sequentially by pytest hooks in a single process — no new threads/async — so no data-race concerns there). The blocking issues are in the two RCCL test modules, where this PR reverts previously-fixed, documented behavior unrelated to reporting.

🔴 Reverts the documented dmesg-window fix — RCCL error detection silently drops the last minute of each test

cvs/tests/rccl/rccl_perf.py (start_time / end_time) and cvs/tests/rccl/rccl_regression.py (same)

The PR changes start_time/end_time from date +"%a %b %e %H:%M:%S" back to date +"%a %b %e %H:%M" (dropping seconds) and removes the explanatory comment. That comment warned that verify_dmesg_for_errors' node-scraper path treats analysis_range_end as an exclusive cutoff, so a minute-truncated end time silently drops the final minute of the test's dmesg window — real GPU HW errors / segfaults in that last minute go undetected (false PASS).

Fix: restore the :%S seconds precision on both start_time and end_time in both files.

🔴 Reverts till_end_flag=FalseTrue, re-introducing a cross-test false-failure cascade

cvs/tests/rccl/rccl_perf.py and cvs/tests/rccl/rccl_regression.py (verify_dmesg_for_errors(...) call)

The PR flips verify_dmesg_for_errors(phdl, start_time, end_time, till_end_flag=True) and deletes the comment explaining why it must be False: with till_end_flag=True the scan runs from start_time to the end of the dmesg buffer, so an earlier parametrized test's kernel event (e.g. a scatter_perf segfault) repeatedly fails every subsequent collective in the sweep — a known false-failure cascade.

Fix: restore till_end_flag=False in both files so each test is bounded to its own start..end window.

These two reverts look unrelated to the reporting feature (likely an accidental rebase / branch-point regression) and should be dropped from the PR.


Non-blocking (not gating): after this change test_gen_graph no longer writes an RCCL HTML report directly — it only fills cvs_results_dict, and the deck is produced at session finish via generate_rundeck, which returns early when --html/htmlpath is unset. Fine if cvs run always passes --html.

@solaiys

solaiys commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

@hnimra-amd , Please check if the changes in tests/rccl/ is intentional or by mistake.

Comment thread cvs/tests/rccl/conftest.py Outdated
Emit viewer_config from sweep payloads, refactor the interactive viewer to read it, and remove RCCL reference profiles plus in-repo sample generators so suite owners add those locally.
Drop session publishing fixtures from RCCL suites and revert test_gen_graph to the existing AmCharts report path until suite owners add profiles.
Remove suite-specific JSON profiles and hooks from this branch. Engine tests use an inline generic sweep profile until dev/dtni lands suite profiles.
Extract publish helpers, remove duplicate inference publish shims, fix JSON profile resolution for pytest extras, and rewrite the platform-only README.
@hnimra-amd
hnimra-amd requested a review from cijohnson August 5, 2026 20:34
hnimra-amd and others added 2 commits August 5, 2026 14:11
Restore per-test dmesg window semantics (:%S timestamps, till_end_flag=False) that were accidentally reverted when RCCL Run Deck wiring was removed. Also fix ruff formatting in publish_helpers, drop an unused import in pytest_extras, and remove internal dev/dtni notes from the Run Deck README.
- Replace functional render_cell_card_html() with CellCardConfig + CellCardRenderer
- Eliminate code duplication and consolidate all logic in CellCardRenderer class
- Update all call sites (pytest_extras, rundeck/cards, tests) to use OO API
- Improve performance for Run Deck platform which renders many cells
- Maintain backward compatibility for cell_card_css() function (still used)

Benefits:
- Single source of truth for cell card rendering logic
- Better performance through renderer reuse (important for Run Deck)
- Clean separation of configuration and rendering
- Eliminated functional API reduces complexity

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Ignatious Johnson <ichristo@amd.com>
@cijohnson
cijohnson force-pushed the hnimrama/rundeck-reports branch from f65d4ed to 2e1eae0 Compare August 5, 2026 23:57

@cijohnson cijohnson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @hnimra-amd Overall the json interface and framework look good.

i have couple of minor comments related to rccl changed, also i have following recommendation.

  1. unittests are using pytest, we decided to used unittest framework for UT's and pytest for the actual test suites. so that the run_all_unittests.py can discover only the unitests based UT's and run during "make test". Also the UT's has to be under respective dirs. refer AGENTS.md (if you can instruct your coding agent to refer AGENTS.md while writing UT's it will take care of it for you). Can you migrate the UT's to unittest framework please

  2. I see functional programming style used in this cvs/report/* library, Can you please move to Object Oriented design, so that we no need to pass around a lot of args.
    I have modified cvs/lib/report/render/cell_card.py to use Object Oriented design and have come with it own UT in this commit (2e1eae0 in your branch, you can use that as a reference and improve on it.

Convert all cvs/lib/report unit tests from pytest to unittest.TestCase and relocate them beside the modules they cover (rundeck/, viewer/, panels/, render/). Move shared fixtures to testing/fixtures.py so run_all_unittests.py discovers them via make ut.

Refactor Run Deck publish path to object-oriented builders following the CellCardRenderer pattern: ViewerConfigBuilder, RundeckPayloadBuilder, SummaryMetaApplier, and RundeckPublisher. Module-level functions remain as thin wrappers for backward compatibility.
Use a nonexistent builder id for the missing-builder registry test, resolve cvs_results_dict in the pytest-hooks fixture mock, and wire generic_inference_report_config to the default run card builder so provenance link assertions match rendered HTML.
@hnimra-amd
hnimra-amd requested a review from cijohnson August 6, 2026 14:17
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