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

[Bug] Temporal Framework Python interface does not take into account changes of mapset #629

Open
lrntct opened this issue May 12, 2020 · 7 comments
Labels
bug Something isn't working

Comments

@lrntct
Copy link
Contributor

lrntct commented May 12, 2020

Describe the bug
When changing the mapset inside a Python script, the temporal framework does not take this change into account and fail to connect to the temporal database, even thought GRASS reports the new mapset as being accessible.

To Reproduce

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import tempfile

import grass_session
import grass.script as gscript

session = grass_session.Session()
tmpdir = tempfile.mkdtemp()
session.open(gisdb=tmpdir,
             location='xy',
             mapset=None,
             create_opts='XY',
             loadlibs=True)

import grass.temporal as tgis

def do_something():
    tgis.init()
    tlist = gscript.read_command('t.list')
    print(tlist)

gscript.run_command('g.mapset', mapset='test1', flags='c')
print(gscript.read_command('g.mapset', flags='p'))
print(gscript.read_command('g.mapsets', flags='p'))
do_something()

gscript.run_command('g.mapset', mapset='test2', flags='c')
print(gscript.read_command('g.mapset', flags='p'))
print(gscript.read_command('g.mapsets', flags='p'))
do_something()

Output:


Loading libraries from /usr/lib/grass78/lib
Mapset switched. Your shell continues to use the history for the old mapset
You can switch the history by commands:
history -w; history -r /tmp/tmp7dc70ya4/xy/test1/.bash_history;
HISTFILE=/tmp/tmp7dc70ya4/xy/test1/.bash_history
test1

Accessible mapsets:
test1 PERMANENT

Default TGIS driver / database set to:
driver: sqlite
database: $GISDBASE/$LOCATION_NAME/$MAPSET/tgis/sqlite.db
WARNING: Temporal database connection defined as:
         /tmp/tmp7dc70ya4/xy/test1/tgis/sqlite.db
         But database file does not exist.
Creating temporal database: /tmp/tmp7dc70ya4/xy/test1/tgis/sqlite.db
----------------------------------------------

Mapset switched. Your shell continues to use the history for the old mapset
You can switch the history by commands:
history -w; history -r /tmp/tmp7dc70ya4/xy/test2/.bash_history;
HISTFILE=/tmp/tmp7dc70ya4/xy/test2/.bash_history
test2

Accessible mapsets:
test2 PERMANENT

Default TGIS driver / database set to:
driver: sqlite
database: $GISDBASE/$LOCATION_NAME/$MAPSET/tgis/sqlite.db
ERROR: Unable to execute sql statement. You have no permission to access
mapset <test2>, or mapset <test2> has no temporal database. Accessible
mapsets are: <test1>

Expected behavior
I would expect the GRASS temporal framework to take into account the changes of the current mapset. This could be circumvented by the use of subprocess, but it is not convenient.

System description (please complete the following information):

  • Operating System: Ubuntu 20.04
  • GRASS GIS version: 7.8.2

Additional context
Perhaps related:
https://trac.osgeo.org/grass/ticket/2110
https://trac.osgeo.org/grass/ticket/3668

@zarch
Copy link
Contributor

zarch commented May 13, 2020

Hi @lrntct ,
I try to follow the links and the discussions that you mentioned, it is still not clear to me why running the code in a new process with subprocess fix the issue.
Do you understand what is it wrong/missing?

@lrntct
Copy link
Contributor Author

lrntct commented May 13, 2020

@zarch I do not know much of the internals of the temporal framework, but it seems that it sets several global variables at invocation:

# We need to access the current mapset quite often in the framework, so we make
# a global variable that will be initiated when init() is called
current_mapset = None
current_location = None
current_gisdbase = None

Maybe some of them are not properly cleaned up or reset when changing the mapset?

@wenzeslaus
Copy link
Member

@zarch Running them in a subprocess should fix it because the init is done again.

@lrntct A reasonable workaround seems to be using the modules (through grass.script or grass.pygrass) rather than the functions. This would be more natural in GRASS context and likely more convenient than managing that through subprocesses manually.

That's not to say that the current library behavior is great, okayish perhaps.

@lrntct
Copy link
Contributor Author

lrntct commented May 14, 2020

@wenzeslaus I understand that the GRASS scripting library runs each command in a subprocess.
Calling modules is not very pythonic (expecting files instead of objects, etc.), and if the code makes many modules calls, it will likely be a performance hit of spawning a process each time.

@wenzeslaus
Copy link
Member

@lrntct I agree. That's why I'm saying it's a workaround.

Just based on your note about the current implementation: The global state in the library is not very Pythonic either which is the root of the issue. Perhaps useful as a default, but some different approach is needed like an way to create a new, different object for the connection to be more flexible in what can be done.

@lrntct
Copy link
Contributor Author

lrntct commented May 15, 2020

@wenzeslaus I wonder if encapsulating those functions and variables in a class could be possible, because it might help solve the issue. It would be a major refactor because those global variables are almost everywhere in core.py. It would be interesting to have the opinion of Sören on the matter, if he's still around.

@wenzeslaus
Copy link
Member

Yes, that's a path I would try to go. Some kind of session, connection object or environment passed around to the functions (or providing the functions directly) and the functions possibly defaulting to whatever is the current/global environment (although that could be more danger than it is worth). Something similar is now possible for some functions in grass.script which take env.

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

No branches or pull requests

3 participants