grass.pygrass: Reset back mapset search path after GridModule class instance finish

[tmszi]

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 OSGeo/grass-addons#787.

[tmszi]

Commit 55dfaf4 and f28483a 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.

[wenzeslaus]

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

Sprinkling _reset_mapset_search_path in the code seems tricky to get right.

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.

[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?

[tmszi]

Remove extend current MAPSET search path.

[petrasovaa]

I believe we should keep this. If I interpret this correctly, this is trying to replicate search path of the current mapset in the temporary mapset. So if user doesn't specify input with full name@mapset, this would be needed. At least it doesn't harm, since it's a temp mapset anyway.

[tmszi]

I originally made it so that the user didn't have to specify the exact MAPSET name (full name@MAPSET) when entering the input map name, but then the search path of the current MAPSET needs to be reseted after the run() method completes.

#2567 (review)

[tmszi]

I believe we should keep this. If I interpret this correctly, this is trying to replicate search path of the current mapset in the temporary mapset. So if user doesn't specify input with full name@mapset, this would be needed. At least it doesn't harm, since it's a temp mapset anyway.

Example (if these lines are preserved):

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

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

3. Launch Python script mentioned here
4. Check current MAPSET search path again

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

[petrasovaa]

I think the problem is elsewhere, see #2584.

[tmszi]

Tested with #2584 and these code lines are preserved (which you mentioned above):

4. Check current MAPSET search path again

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

Current MAPSET search path is the same as before running GridModule class instance run() method.

With these preserved code lines (which you mentioned above) for input map name is still required full name map@MAPSET.

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

addressed