Repository navigation
temporal: Avoid PLY debug-mode overhead in raster algebra gunittest - #7903
Merged
echoix merged 1 commit intoSep 8, 2026
Merged
Conversation
unittests_temporal_raster_algebra.py instantiates TemporalRasterAlgebraParser with debug=True in all 43 of its test methods (45 instantiations), far more than any sibling test file in this family (the next highest is 26). With debug=True, the vendored PLY yacc.yacc() (python/grass/temporal/ply/yacc.py) rebuilds and verbosely logs the full LALR table on every call, and opens a parser.out debug file that is never closed (confirmed by a "ResourceWarning: unclosed file ... 'parser.out'" in a macOS CI log), in addition to enabling extra "if self.debug: print(...)" statements throughout temporal_raster_base_algebra.py. None of this output is used by the tests or by CI. This matches a macOS-only CI failure where this file alone timed out at the gunittest per-file limit (FAILED ... Timeout >300.0s) after completing only a fraction of its tests, while the same file runs all 43 tests in about a minute on Linux CI. The production CLI tool (t.rast.algebra.py) already calls this parser with debug=False, and TemporalRasterBaseAlgebraParser's own default is debug=False; only this test file's parser subclass overrode it to True. Passing debug=False here removes avoidable per-test disk I/O and redundant LALR logging that a contended or slow-disk CI runner would feel most, without changing what is tested (no test inspects the debug output). Investigated by pulling recent macOS Actions runs on this fork, reading the gunittest and PLY yacc internals, and comparing timing across CI runs; see PR description for the full analysis. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
Python
ninsbl
approved these changes
Sep 8, 2026
wenzeslaus
approved these changes
Sep 8, 2026
wenzeslaus
left a comment
wenzeslaus
left a comment
Member
There was a problem hiding this comment.
You could just use the default, no specific reason to be explicit, no? Good as it is, too.
echoix
deleted the
claude/macos-raster-algebra-timeout-investigation
branch
September 12, 2026 19:47
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.
Related to #7897, this is the result of AI investigating what could have fixed it. The approach here would be to not write a full debug file for each test (reducing work to do, and avoiding the unclosed file in that code path).
Note that other test files still use debug=True. Investigation on 100 other runs on main (of my fork, but only logs retrievable, so I don’t think it got 100 of them), there was another time where it failed for timeout on that file, where this fix would have had an impact. So, the lower bound on the occurrence would be 1%.
On the test run on my fork, macOS had lower time taken for that file (writing files seems slower now), but Ubuntu 24.04 didn’t, but that machine was slower overall. So, don’t count on faster test times because of that.
Description
unittests_temporal_raster_algebra.pyconstructsTemporalRasterAlgebraParser(..., debug=True)in all 43 of its test methods. That flag makes the vendored PLYyacc.yacc()rebuild and dump the full LALR debug trace toparser.outon every call — measured locally at 2.86 MB per dump, identical every time since the grammar is static, and the file handle is never closed (ResourceWarning: unclosed file ...'parser.out'). None of this is used by the tests. Production code (t.rast.algebra.py) already usesdebug=False, as does the base class's own default; only this test file overrides it.This PR switches all 45 instantiations in that file to
debug=False, matching production usage and removing ~129 MB of redundant writes and 45 leaked file descriptors per test run.Motivation and context
Investigating an intermittent macOS-only CI timeout on this exact test file (
Timeout >300.0s). I can't prove this fixes that specific timeout — comparing recent macOS CI runs shows the same 300s ceiling hit by an unrelated test on a different occasion, which points more to runner-level I/O contention than a deterministic bug, and a local before/after timing comparison showed no measurable difference on a fast disk. This change is a genuine, independently-justified cleanup (dead debug output, a real leaked file handle) that reduces this file's exposure to slow/contended disk, not a targeted fix for the flake.How has this been tested?
Built GRASS from source locally and ran
unittests_temporal_raster_algebra.pydirectly, both before (debug=True) and after (debug=False): 43/43 tests pass both ways, confirming no behavioral change.Types of changes
Checklist
Investigated and fixed with AI assistance (Claude Code); the analysis, the decision of what (not) to fix, and the verification above are mine.