Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

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

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
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.

python/grass/pygrass/modules/grid/grid.py Outdated Show resolved Hide resolved
python/grass/pygrass/modules/grid/grid.py Outdated Show resolved Hide resolved
python/grass/pygrass/modules/grid/grid.py Outdated Show resolved Hide resolved
python/grass/pygrass/modules/grid/grid.py Show resolved Hide resolved
@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
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.

python/grass/pygrass/modules/grid/grid.py Outdated Show resolved Hide resolved
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 added a commit to tmszi/grass that referenced this pull request Nov 15, 2022
@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
marisn pushed a commit to marisn/grass that referenced this pull request Jun 2, 2023
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants