Skip to content

Comments

grass.pygrass: GridModule clean up temporary mapsets when exception occurs#2614

Merged
tmszi merged 6 commits intoOSGeo:mainfrom
tmszi:fix-pygrass-gridmodule-class-clean-temp-mapsets
Nov 15, 2022
Merged

grass.pygrass: GridModule clean up temporary mapsets when exception occurs#2614
tmszi merged 6 commits intoOSGeo:mainfrom
tmszi:fix-pygrass-gridmodule-class-clean-temp-mapsets

Conversation

@tmszi
Copy link
Member

@tmszi tmszi commented Oct 28, 2022

Describe the bug
run() method of the GridModule class instance doesn't clean up temporary MAPSETs when exception occurs (input raster map doesn't exists).

To Reproduce
Steps to reproduce the behavior:

  1. Launch GRASS GIS with nc_spm_08_grass7 LOCATION and PERMANENT MAPSET
  2. Launch this Python script
#!/usr/bin/env python3

from grass.pygrass.modules.grid.grid import GridModule

def main():
    grd = GridModule("r.slope.aspect",
                    width=100, height=100, overlap=2,
                    processes=None, split=False, elevation="non_exist_elevation", 
                    slope="slope", aspect="aspect", overwrite=True)
    grd.run()

if __name__ == '__main__':
    main()
  1. See error
GRASS nc_spm_08_grass7/PERMANENT:grass > python /tmp/test.py 
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
ERROR: Raster map <non_exist_elevation> not found
Traceback (most recent call last):
  File "/tmp/test.py", line 13, in <module>
    main()
  File "/tmp/test.py", line 10, in main
    grd.run()
  File "/usr/lib64/grass83/etc/python/grass/pygrass/modules/grid/grid.py", line 675, in run
    self.patch()
  File "/usr/lib64/grass83/etc/python/grass/pygrass/modules/grid/grid.py", line 712, in patch
    rpatch_map(
  File "/usr/lib64/grass83/etc/python/grass/pygrass/modules/grid/patch.py", line 94, in rpatch_map
    rtype.open("r")
  File "/usr/lib64/grass83/etc/python/grass/pygrass/raster/__init__.py", line 218, in open
    raise OpenError(str_err)
grass.exceptions.OpenError: The map does not exist, I can't open in 'r' mode
  1. Check the list of LOCATION MAPSETs
GRASS nc_spm_08_grass7/PERMANENT:grass > tree -d -L 1 $(g.gisenv get=GISDBASE)/$(g.gisenv get=LOCATION_NAME)
/home/tomas/grassdata/nc_spm_08_grass7
├── climate_2000_2012
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_000_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_000_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_000_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_000_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_000_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_000_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_001_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_001_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_001_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_001_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_001_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_001_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_002_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_002_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_002_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_002_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_002_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_002_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_003_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_003_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_003_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_003_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_003_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25275_003_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_000_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_000_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_000_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_000_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_000_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_000_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_001_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_001_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_001_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_001_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_001_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_001_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_002_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_002_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_002_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_002_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_002_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_002_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_003_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_003_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_003_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_003_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_003_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_003_005
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_004_000
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_004_001
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_004_002
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_004_003
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_004_004
├── grid_r_slope_aspect_gentoo_gnu_linux_25704_004_005
├── landsat
├── PERMANENT
└── tomas

Expected behavior
Clean up the temporary MAPSETs when an exception (input raster map does not exist) occurs during the run() method of an instance of the GridModule class.

System description (please complete the following information):

  • Operating System: all
  • GRASS GIS version: all

@tmszi
python/grass/pygrass/modules/grid: clean up the temporary MAPSETs
2ced32d 
When an exception (input raster map does not exist) occurs during the
`run()` method of an instance of the `GridModule` class.
@tmszi tmszi added bug Something isn't working backport_needed labels Oct 28, 2022
@tmszi tmszi modified the milestones: 8.3.0, 8.2.1 Oct 28, 2022
@petrasovaa petrasovaa requested a review from wenzeslaus November 2, 2022 04:52
wenzeslaus
wenzeslaus requested changes Nov 9, 2022
View reviewed changes
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this separately from the other issue. I think the general idea is good, but some changes would make it better - see comments.

I think a test would be doable for the specific bug being fixed. pytest seems like a good choice for that. The "positive" case where everything works is probably easier to test with grass.gunittest. There are some doctest-based tests for GridModule, but I don't know how good they are and there seems to be some confusion between tests of grass.pygrass.modules and grass.pygrass.modules.grid.

Generally, it seems that the resource handling should be more clear. The resource allocation and cleanup should be linked together in a better way, e.g., by having the cleanup set up where the resources are allocated. This may overall improve the class, but for fixing the bug, the current approach in this PR is enough.

@wenzeslaus wenzeslaus changed the title python/grass/pygrass/modules/grid: clean up the temporary MAPSETs when exception occurs (input raster map does not exist) grass.pygrass: GridModule clean up temporary mapsets when exception occurs Nov 9, 2022
@tmszi tmszi requested a review from wenzeslaus November 11, 2022 17:54
wenzeslaus
wenzeslaus requested changes Nov 15, 2022
View reviewed changes
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

This is great - short and sweet!

I have two comments, the test actually requires some attention.

wenzeslaus
wenzeslaus approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

Great!

@tmszi tmszi merged commit 15a01ef into OSGeo:main Nov 15, 2022
tmszi added a commit to tmszi/grass that referenced this pull request Nov 15, 2022
@tmszi
grass.pygrass: GridModule clean up temporary mapsets when exception o…
42b39e8 
…ccurs (OSGeo#2614)
tmszi added a commit to tmszi/grass that referenced this pull request Nov 15, 2022
@tmszi
grass.pygrass: GridModule clean up temporary mapsets when exception o…
3a30748 
…ccurs (OSGeo#2614)
@tmszi tmszi deleted the fix-pygrass-gridmodule-class-clean-temp-mapsets branch November 15, 2022 06:53
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
@tmszi @ninsbl
grass.pygrass: GridModule clean up temporary mapsets when exception o…
793d11c 
…ccurs (OSGeo#2614)
marisn pushed a commit to marisn/grass that referenced this pull request Jun 2, 2023
@tmszi @marisn
grass.pygrass: GridModule clean up temporary mapsets when exception o…
9d6bced 
…ccurs (OSGeo#2614)
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
@tmszi @neteler
grass.pygrass: GridModule clean up temporary mapsets when exception o…
19a2a0e 
…ccurs (OSGeo#2614)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wenzeslaus wenzeslaus wenzeslaus approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

8.2.1

Development

Successfully merging this pull request may close these issues.

2 participants

@tmszi @wenzeslaus