Repository navigation
ai-summary: ignore errors a test declared expected - #133
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the AI log extraction pipeline to suppress developer-declared expected error lines (via EXPECTED_ERROR log brackets) before downstream crash detection and error section extraction, reducing false “CRASHED” classifications for negative tests.
Changes:
- Added parsing + masking for
[EXPECTED_ERROR BEGIN ... message=...] ... [EXPECTED_ERROR END]blocks so declared expected error lines don’t influence crash detection or extracted error sections. - Refactored crash detection into clearer buckets (hard crash / collection crash / killed / TT_FATAL+TT_THROW / selected Python exceptions).
- Applied expected-error masking early in
extract_log()so it affects all later processing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+283
to
+285
| """Line ranges of properly closed [EXPECTED_TT_ERROR BEGIN match=...]...[END] | ||
| blocks, paired with the declared match pattern. An unclosed block is ignored, | ||
| so its contents are never masked.""" |
Comment on lines
+300
to
+304
| """Return a copy of `lines` with developer-declared expected errors blanked out. | ||
| For each closed [EXPECTED_TT_ERROR BEGIN match='<pat>']...[END] bracket, the two | ||
| marker lines and any in-between line matching <pat> are replaced with "" — so they | ||
| are seen by neither crash detection nor section extraction. Everything else — a | ||
| different line in the block, or anything outside a bracket — is untouched.""" |
Comment on lines
+310
to
+311
| masked[start] = "" # [EXPECTED_TT_ERROR BEGIN ...] marker | ||
| masked[end] = "" # [EXPECTED_TT_ERROR END] marker |
Comment on lines
+432
to
+434
| # Strip developer-declared expected errors (expect_tt_error brackets) up front, so | ||
| # they're excluded from EVERYTHING downstream — crash detection and the error | ||
| # sections handed to the LLM alike. |
Comment on lines
+450
to
+452
| # TT_THROW/TT_FATAL means tt-metal crashed, which cascades to vLLM timeout and test | ||
| # failures. (full_text already has expect_tt_error-declared errors masked out above.) | ||
| # |
Comment on lines
+432
to
+436
| # Strip developer-declared expected errors (expect_tt_error brackets) up front, so | ||
| # they're excluded from EVERYTHING downstream — crash detection and the error | ||
| # sections handed to the LLM alike. | ||
| lines = _mask_expected_errors(lines) | ||
|
|
vmilosevic
previously approved these changes
Jun 15, 2026
ppetrovicTT
force-pushed
the
ppetrovic/ai-summary-negative-test
branch
from
June 19, 2026 10:37
127d1b9 to
d0ac90d
Compare
ppetrovicTT
force-pushed
the
ppetrovic/ai-summary-negative-test
branch
from
June 19, 2026 11:44
d0ac90d to
54294a7
Compare
Contributor
There was a problem hiding this comment.
⚠️ Not ready to approve
The current masking implementation can drop newlines (risking cross-line regex matches), and the new behavior around error section extraction isn’t fully asserted by the added tests.
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 5
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Comment on lines
+351
to
+360
| for start, block_end, pattern in ranges: | ||
| masked[start] = "" # [EXPECTED_ERROR BEGIN ...] marker | ||
| masked[block_end] = "" # [EXPECTED_ERROR END] marker | ||
| try: | ||
| pat = re.compile(pattern) | ||
| except re.error: | ||
| continue # malformed pattern → mask only the markers, treat content as real | ||
| for i in range(start + 1, block_end): | ||
| if pat.search(masked[i]): | ||
| masked[i] = "" |
| nonempty = [ln for ln in lines if ln.strip()] | ||
| completion_chunks = [(log_source.name, nonempty[:_MARKER_WINDOW], nonempty[-_MARKER_WINDOW:])] | ||
|
|
||
| # Strip developer-declared expected errors (expect_tt_error brackets) up front, so |
Comment on lines
+89
to
+95
| def test_declared_error_is_masked(self, tmp_path): | ||
| log = ( | ||
| '[EXPECTED_ERROR BEGIN] RuntimeError message="must be BFLOAT8_B"\n' | ||
| "TT_FATAL global_tensor must be BFLOAT8_B or BFLOAT16, got Float32\n" | ||
| '[EXPECTED_ERROR END] RuntimeError message="must be BFLOAT8_B"\n' | ||
| ) | ||
| assert not _extract(tmp_path, log).has_crash |
Comment on lines
+97
to
+103
| def test_undeclared_error_in_block_still_crashes(self, tmp_path): | ||
| log = ( | ||
| '[EXPECTED_ERROR BEGIN] RuntimeError message="must be BFLOAT8_B"\n' | ||
| "TT_FATAL unrelated device hang\n" | ||
| '[EXPECTED_ERROR END] RuntimeError message="must be BFLOAT8_B"\n' | ||
| ) | ||
| assert _extract(tmp_path, log).has_crash |
Comment on lines
491
to
495
| detection_patterns: dict | None = None, | ||
| expected_error_markers: dict | None = None, | ||
| ) -> ExtractedLog: | ||
| """ | ||
| Extract important parts from a CI log. |
Lines inside an [EXPECTED_ERROR BEGIN <Type> message="..."]...[END] bracket (emitted by the expect_error pytest fixture) that match the declared message are masked before crash detection AND section extraction. A deliberately provoked error (e.g. a negative test's TT_FATAL) no longer triggers a false CRASH, while a different error in the same block — or anything outside a bracket — still does. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A pytest SKIPPED result never ran, so a crash/timeout token quoted in its skip reason (e.g. "Disabled by #44858: ... TT_FATAL ...") was flipping the job to CRASHED. Blank lines matching ignored_line_patterns (analysis.yaml) up front, alongside the expected-error masking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ppetrovicTT
force-pushed
the
ppetrovic/ai-summary-negative-test
branch
from
June 19, 2026 13:13
54294a7 to
d648faa
Compare
Two pre-detection passes (expected-error brackets, non-event lines) and the window+timestamp gate are non-obvious; record the flow and the invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
✅ Ready to approve
The change is well-scoped, includes solid test coverage for the new masking behavior, and the remaining feedback is limited to small documentation/performance nits.
Note: this review does not count toward required approvals for merging.
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 3
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
Comment on lines
+309
to
+313
| def _default_expected_error_markers() -> dict: | ||
| """Bundled expected_error_markers, for direct extract_log() calls without config.""" | ||
| from .config import load_config | ||
|
|
||
| return load_config().get("expected_error_markers", {}) |
Comment on lines
+316
to
+320
| def _default_ignored_line_patterns() -> list[str]: | ||
| """Bundled ignored_line_patterns, for direct extract_log() calls without config.""" | ||
| from .config import load_config | ||
|
|
||
| return load_config().get("ignored_line_patterns", []) |
Copilot review: one leftover expect_tt_error comment (markers are expect_error), and extract_log's Args omitted detection_patterns/expected_error_markers/ ignored_line_patterns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lines inside an [EXPECTED_ERROR BEGIN message="..."]...[END] bracket (emitted by the expect_error pytest fixture) that match the declared message are masked before crash detection AND section extraction. A deliberately provoked error (e.g. a negative test's TT_FATAL) no longer triggers a false CRASH, while a different error in the same block — or anything outside a bracket — still does.