python/grass/pygrass: reset back MAPSET search path after GridModule class instance finish

[tmszi]

trafficstars

Describe the bug Python pygrass GridModule class instance change LOCATION MAPSET search path.

To Reproduce Steps to reproduce the behavior:

1. Launch GRASS GIS with nc_spm_08_grass7 LOCATION and landsat MAPSET
2. Launch g.mapsets operation=set mapset=landsat

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -p
Accessible mapsets:
landsat

4. 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="elevation", 
                    slope="slope", aspect="aspect", overwrite=True)
    grd.run()

if __name__ == '__main__':
    main()

5. Check output of g.mapsets -p

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -p
Accessible mapsets:
landsat PERMANENT climate_2000_2012

Expected behavior Python pygrass GridModule class instance should not change LOCATION MAPSET search path.

System description (please complete the following information):

• Operating System: all
• GRASS GIS version: all

Additional context

GRASS GIS Addons issue https://github.com/OSGeo/grass-addons/pull/787.

[tmszi]

Commit https://github.com/OSGeo/grass/pull/2567/commits/55dfaf4c5b276c9383654d06fc5469886aaa5ddc and https://github.com/OSGeo/grass/pull/2567/commits/f28483ac63b5e243e2895e451ec6703142ad2c40 handle situation (reset back MAPSET search path and clean temp data) if input map doesn't exists (example bellow basin raster map doesn't exists in the current MAPSET) and OpenError exception is raised.

Example:

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets operation=set mapset=landsat
GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -p
Accessible mapsets:
landsat

GRASS nc_spm_08_grass7/landsat:~ > g.extension r.mapcalc.tiled

GRASS nc_spm_08_grass7/landsat:~ > r.mapcalc.tiled expression="test = if(basin==12, 1, null())" overlap=2 width=200 height=200
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_000_000
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_000_001
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_000_002
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_001_000
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_001_001
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_001_002
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_002_000
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_002_001
Invalid map <basin>
Parse error
ERROR: parse error
DEST grid_r_mapcalc_gentoo_gnu_linux_9399_002_002
Invalid map <basin>
Parse error
ERROR: parse error
Traceback (most recent call last):
  File "/usr/lib64/grass83/etc/python/grass/pygrass/modules/grid/grid.py", line 686, in run
    self.patch()
  File "/home/tomas/.grass8/addons/scripts/r.mapcalc.tiled", line 124, in patch
    rpatch_map(
  File "/usr/lib64/grass83/etc/python/grass/pygrass/modules/grid/patch.py", line 95, 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

GRASS nc_spm_08_grass7/landsat:~ > g.mapsets -p
Accessible mapsets:
landsat

[tmszi]

With this PR GridModule class instance internally extend current MAPSET search path by all accesible LOCATION MAPSETs.

Example:

When input map name e.g. elevation has unique name across all LOCATION MAPSETs you could use simple map name as input param e.g. elevation="elevation" in r.slope.aspect module (Python code example in the comment above). However if elevation map isn't unique across all LOCATION MAPSETs you have to use fully qualified map name elevation="elevation@climate_2000_2012" in r.slope.aspect module, because you get warning message. And in the case that you want use different input elevation map than the one is in current MAPSET.

Using <elevation@PERMANENT>...
Data element 'cellhd/elevation' was found in more mapsets (also found in <landsat>)
Data element 'cellhd/elevation' was found in more mapsets (also found in <climate_2000_2012>)

On the end of GridModule class instance run processing it reset back current MAPSET search path to the state before running this process.

[tmszi]

I think GridModule should not modify the search path of the current mapset at all. It should simply use fully qualified names if needed.

Yes, I agree with you.

try-except with explicit traceback handling seems wrong. Why not finally or using context manager and with? But again, no reset would be needed if no modification is done which seems to be the most robust solution.

Yes, you're right, thanks.

[wenzeslaus]

Can you please extend the description to comment on what actually changed and what is just indentation?

[petrasovaa]

This is mixing a bugfix and enhancement, so it seems to me the CleanManager should be separated into another PR. There have been changes (#2584) and I didn't test recently but it seems only the change in patch() is needed to address the original bug (modifying search path), or?

[tmszi]

This is mixing a bugfix and enhancement, so it seems to me the CleanManager should be separated into another PR. There have been changes (#2584) and I didn't test recently but it seems only the change in patch() is needed to address the original bug (modifying search path), or?

Yes, you are right.

There are two bugs:

1. patch() method expose all LOCATION MAPSETs, fixed with https://github.com/OSGeo/grass/pull/2567/commits/e88236fb62cfb0bf9d0ac99f8e2ce710057773a0.
2. If input map doesn't exists, temporary MAPSETs are not cleaned automatically (solved with CleanManager class). I will open new PR.

[petrasovaa]

It looks like the grid.py tests are broken, but I don't see why this change would trigger that. Anyway, the doctest probably needs to be fixed, it shouldn't require SEARCH_PATH.

[tmszi]

It looks like the grid.py tests are broken, but I don't see why this change would trigger that.

I think this is because the SEARCH_PATH file does not exist by default because we removed this line mset.visible.extend(loc.mapsets()) which creates this file.