[cgmillette]

Motivation

The Python style guide said little about how code should report bad input: one sentence on ValueError vs NotImplementedError and a one-line testing reminder. Authors and coding agents had no shared rule for which exception to raise, how to word the message, where checks belong, or how a Python check lines up with the C++ engine's error channel.

Applying the parity rule found one pair that did not match: the C++ QkScaleSpec validation dropped the bad value the Python messages name. That is fixed here too.

ISSUE ID : AICK-1705

Technical Details

• New section 13 "Error handling" in style/PYTHON_STYLE.md, written as discrete, self-contained rules, with one sample rule in a RULE / WHEN / DO / DON'T / WHY form:
  • Principles: fail fast at the public boundary; checks never emit IR and run once, not inside emission loops; contracts enforced at runtime; a warning that a new check may break a caller that relied on undefined behaviour (intended).
  • Message template {what failed} - {param}={bad value}, expected {constraint}, with a good and a banned example.
  • Exception table: ValueError (default), TypeError, KeyError, NotImplementedError, RuntimeError (and the existing ComgrError / HipError subclasses); never assert for public input validation.
  • Placement: __post_init__ for field invariants, is_valid_spec for arch-dependent checks, when a private helper may skip, duck typing over blanket isinstance, numeric parameters first.
  • Parity with the C++ engine, shown on a real Python/C++ pair (rocke_i_set_err, which throws through the extern "C" guard), and the exception-to-status-code mapping; the C++ side stays owned by CPP_STYLE.md section 6.
  • Mandatory error-condition tests pinning the exception type and the message, with a before/after example.
  • Type hints do not replace runtime checks.
• Sections 7 and 11 and the checklist now point to section 13 instead of restating exception choice and rejection testing.
• Policy only: bringing existing code in line is tracked separately.
• rocke_qk_scale_spec_validate (C++) now writes the Python QkScaleSpec message, bad value included, into a caller buffer (reason, reason_cap), the convention other C++ validators use; it used to return fixed strings without the value. The layout is an enum in C, so an unknown layout is reported by its integer value. The function had no callers. No LLVM IR changes.

Test Plan

• Ran the section's test example against the current package.
• Checked every cited symbol, file and status code against the current code.
• Checked that the anchors other docs link to (sections 7, 8, 11) are unchanged.
• pre-commit over the change.
• tests/core/test_qk_scale_spec.py (the section's "after" example as a real test) pins the Python rejections; the new native test rocke_qk_scale_spec_validate, registered in the CTest tiers, pins the same texts in the C++ engine.
• Byte identity at llvm20, llvm22, llvm23; ctest; full platform suite.

Test Result

• The test example passes.
• All cited symbols exist as described; existing anchors unchanged.
• pre-commit passes.
• Both QkScaleSpec tests pass; byte identity green at llvm20, llvm22 and llvm23 (no IR change); ctest passes; full platform suite passes with no failures.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests.

🤖 Generated with Claude Code

[therock-pr-bot]

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🔎 pre-commit	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—
🤖 therock-pr-bot	✅ Pass	—

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

[therock-pr-bot]

🎉 All checks passed! This PR is ready for review.

[cgmillette]

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #13964 — add an error-handling policy to the Python style guide — 🟢 First-round: no closer look needed — policy + safe ABI change, parity tests pass

Prior item	Status
Reviewers	🟢 None yet — only the policy bot
Bots / CI	🟢 Policy bot, pre-commit, clang-tidy, TheRock CI, Multi-Arch and hipDNN superbuild all green. MERGEABLE
Our checks	🟢 Clean merge with develop; rule_scan clean (ran). Python parity test test_qk_scale_spec.py: 4 passed (ran). Native-built the head and ran the C++ ctest rocke_...qk_scale_spec_validate: passed (ran — this is the half the local agent can't do). The rocke_qk_scale_spec_validate signature change (const char** out_reason → char* reason, size_t reason_cap) is ABI-safe: grep confirms no callers outside its declaration and test (ran).

Net-new: none.

Utility: 🟢 Positive — adds a concrete, example-driven error-handling policy (exception choice, message templates, validation placement, C++/Python parity, mandatory error tests) and lands a matching caller-free ABI tweak with Python+C++ parity tests.

---

Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

First-round review: a local qwen3.6 model made the independent first pass; the orchestrator (Claude Opus 4.8) verified it on hardware — native build + C++ ctest + caller analysis — and owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

[CongMa13]

No new issues found.

[CongMa13]

No new P0 or P1 issues were identified during this review. The changes are approved.

[assistant-librarian]

TheRock Submodule Bump Activity

Newest first

• 2026-10-22 — Included in TheRock via ROCm/TheRock#9426.