Skip to content

grass.pygrass: Close RasterRow via context manager in region tests - #686

Closed
echoix wants to merge 1 commit into
mainfrom
claude/raster2numpy-region-timing-fix
Closed

grass.pygrass: Close RasterRow via context manager in region tests#686
echoix wants to merge 1 commit into
mainfrom
claude/raster2numpy-region-timing-fix

Conversation

@echoix

@echoix echoix commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Description

test_raster_region.py's test_resampling_1 and test_resampling_2 called
rast.close() as a bare statement after their assertions. If an assertion
(or anything else) raised, that close() call was skipped and the
RasterRow stayed open for the rest of the process.

Region.set_raster_region() documents exactly why that matters:

Attention: All raster objects must be closed or the process will be
terminated.

test_resampling_to_numpy, the next test in this file, calls
region.set_raster_region() on every resampling step. So a raster left open
by an earlier test doesn't just fail that earlier test — it crashes the
next test with an unrelated-looking C-level FERROR: Input window changed while maps are open for read, instead of whatever the real original failure
was.

Fixed by using with rast: (RasterRow already documents and supports this)
instead of a manual open()/close() pair, so the raster is always closed,
assertion failure or not.

Known follow-up

Windows CI on this branch shows the fatal, whole-file-crashing FERROR is
gone, but the file still doesn't fully pass: three individual failures show
up instead (count/value mismatches in test_resampling_1 and
test_resampling_2, and an oversized array allocation in
test_resampling_to_numpy). These point to a separate, pre-existing bug
where the small test region doesn't appear to actually restrict what gets
read back on Windows. I haven't been able to reproduce it without a Windows
machine, so I'm not attempting a fix for it here — this PR only fixes the
resource leak that was masking it.

Motivation and context

Found while investigating Windows CI test-order-dependency:
test_raster_region.py was one of a small set of consistently-failing
Windows gunittest files.

How has this been tested?

Windows CI on this branch (see the follow-up note above) — I couldn't
reproduce or build GRASS on Windows locally.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • PR title provides summary of the changes and starts with one of the
    pre-defined prefixes
  • My code follows the code style
    of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@echoix
echoix marked this pull request as ready for review September 5, 2026 00:46
@echoix echoix mentioned this pull request Sep 5, 2026
Closed
3 of 6 tasks
@echoix
echoix force-pushed the claude/raster2numpy-region-timing-fix branch from 80fc75c to c4916ec Compare September 5, 2026 02:59
@echoix echoix changed the title pygrass: Close RasterRow via context manager in region tests grass.pygrass: Close RasterRow via context manager in region tests Sep 5, 2026
echoix pushed a commit that referenced this pull request Sep 5, 2026
@claude
grass.pygrass: Mark test_raster_region resampling tests xfail on Windows
800a40f 
Follow-up to #686: fixing the resource leak there turned a fatal,
whole-file-crashing FERROR into three individually diagnosable
failures instead. The remaining issue is real but distinct from the
leak: the small region these tests set up isn't actually restricting
what RasterRow reads back (rast[0] comes back with roughly 100000
elements instead of 10).

Built GRASS locally and confirmed this is genuinely Windows-specific,
not a state leak this environment can reproduce: the file passes
reliably on Linux both run in isolation and as part of the full
359-file gunittest suite in sequence. Marking xfail on Windows per the
existing convention rather than guessing at a fix I can't verify;
needs investigation with actual Windows access.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B9zVtsN8mY7K5GCrpYwjHW
@echoix echoix mentioned this pull request Sep 5, 2026
Closed
4 of 6 tasks
echoix pushed a commit that referenced this pull request Sep 5, 2026
@claude
grass.pygrass: Mark test_raster_region resampling tests xfail on Windows
6c37419 
Follow-up to #686: fixing the resource leak there turned a fatal,
whole-file-crashing FERROR into three individually diagnosable
failures instead. The remaining issue is real but distinct from the
leak: the small region these tests set up isn't actually restricting
what RasterRow reads back (rast[0] comes back with roughly 100000
elements instead of 10).

Built GRASS locally and confirmed this is genuinely Windows-specific,
not a state leak this environment can reproduce: the file passes
reliably on Linux both run in isolation and as part of the full
359-file gunittest suite in sequence. Marking xfail on Windows per the
existing convention rather than guessing at a fix I can't verify;
needs investigation with actual Windows access.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B9zVtsN8mY7K5GCrpYwjHW
@echoix
echoix force-pushed the claude/raster2numpy-region-timing-fix branch 2 times, most recently from 4f13a2d to fee78ab Compare September 5, 2026 21:08
@echoix @claude
grass.pygrass: Close RasterRow via context manager in region tests
0a385ac 
test_resampling_1 and test_resampling_2 called rast.close() as a bare
statement after their assertions, so a failing assertion (or any other
exception) would skip it, leaving the RasterRow open. This repo's own
Region.set_raster_region() documents the consequence: "All raster
objects must be closed or the process will be terminated." The next
test in this file, test_resampling_to_numpy, calls exactly that method
on every resampling step, so an open handle carried over from an
earlier test crashes it with an unrelated-looking C-level FERROR
instead of the real assertion failure ever being reported.

Use `with rast:` instead, matching RasterRow's documented context
manager support, so the raster is always closed, including when an
assertion fails.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@echoix
echoix force-pushed the claude/raster2numpy-region-timing-fix branch from fee78ab to 0a385ac Compare September 5, 2026 21:09
echoix pushed a commit that referenced this pull request Sep 5, 2026
@claude
grass.pygrass: Mark test_raster_region resampling tests xfail on Windows
12843ef 
Follow-up to #686: fixing the resource leak there turned a fatal,
whole-file-crashing FERROR into three individually diagnosable
failures instead. The remaining issue is real but distinct from the
leak: the small region these tests set up isn't actually restricting
what RasterRow reads back (rast[0] comes back with roughly 100000
elements instead of 10).

Built GRASS locally and confirmed this is genuinely Windows-specific,
not a state leak this environment can reproduce: the file passes
reliably on Linux both run in isolation and as part of the full
359-file gunittest suite in sequence. Marking xfail on Windows per the
existing convention rather than guessing at a fix I can't verify;
needs investigation with actual Windows access.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B9zVtsN8mY7K5GCrpYwjHW
echoix added a commit that referenced this pull request Sep 5, 2026
@echoix @claude
grass.pygrass: Mark test_raster_region resampling tests xfail on Windows
0c30f10 
Follow-up to #686: fixing the resource leak there turned a fatal,
whole-file-crashing FERROR into three individually diagnosable
failures instead. The remaining issue is real but distinct from the
leak: the small region these tests set up isn't actually restricting
what RasterRow reads back (rast[0] comes back with roughly 100000
elements instead of 10).

Built GRASS locally and confirmed this is genuinely Windows-specific,
not a state leak this environment can reproduce: the file passes
reliably on Linux both run in isolation and as part of the full
359-file gunittest suite in sequence. Marking xfail on Windows per the
existing convention rather than guessing at a fix I can't verify;
needs investigation with actual Windows access.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@echoix echoix closed this Sep 6, 2026
echoix added a commit that referenced this pull request Sep 6, 2026
@echoix @claude
grass.pygrass: Mark test_raster_region resampling tests xfail on Windows
a8b514d 
Follow-up to #686: fixing the resource leak there turned a fatal,
whole-file-crashing FERROR into three individually diagnosable
failures instead. The remaining issue is real but distinct from the
leak: the small region these tests set up isn't actually restricting
what RasterRow reads back (rast[0] comes back with roughly 100000
elements instead of 10).

Built GRASS locally and confirmed this is genuinely Windows-specific,
not a state leak this environment can reproduce: the file passes
reliably on Linux both run in isolation and as part of the full
359-file gunittest suite in sequence. Marking xfail on Windows per the
existing convention rather than guessing at a fix I can't verify;
needs investigation with actual Windows access.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

libraries Python tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@echoix