Run GRASS with Python3

[zarch]

Dear all,

I've started playing with GRASS and Python3. I did some testing on Python2.6, Python2.7 and Python3.4.

So far I'm able to start a GRASS session and execute something trivial command, like:

python -c "from grass.script import read_command; print(read_command('g.region', flags='p'))"

GRASS under python2 with these changes seems to work fine on my system.

I've splitted the changes in three main patchs:

1) lib/init/grass.py => init_grass.diff

2) lib/python/gunittest/* => gunittest.diff

3) lib/python/script/* => script.diff

My question is how can we share these changes? should we create a separate branch? may I just put these changes on trunk and break it :-D, or should trunk still remain quite stable with not experimental code?

[glynn]

Replying to zarch:

3) lib/python/script/* => script.diff

These are wrong. You should be converting str to bytes, not the other way around. A child process' argv, environment, stdin/stdout/stderr are all byte sequences. Converting everything to unicode so that Python can convert it back to bytes under the hood is adding needless failure modes.

[zarch]

Replying to glynn:

Replying to zarch:

3) lib/python/script/* => script.diff

These are wrong. You should be converting str to bytes, not the other way around. A child process' argv, environment, stdin/stdout/stderr are all byte sequences. Converting everything to unicode so that Python can convert it back to bytes under the hood is adding needless failure modes.

I agree with you, actually this was my first attempt, but then I have to face that the string formatting is available for bytes, only on python 3.5, that it is not stable yet. therefore when I call make_command/run_command I got:

In [1]: from grass.script import core as gcore

In [2]: gcore.make_command('g.region', raster='elevation', flags='p')
Out[2]: ['g.region', '-p', "raster=b'elevation'"]

In [3]: gcore.run_command('g.region', raster='elevation', flags='p')
WARNING: Illegal filename <b'elevation'>. Character <'> not allowed.
ERROR: Raster map <b'elevation'> not found
---------------------------------------------------------------------------
CalledModuleError                         Traceback (most recent call last)
<ipython-input-5-c7b3927bef11> in <module>()
----> 1 gcore.run_command('g.region', raster='elevation', flags='p')

/home/pietro/docdat/src/gis/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/script/core.py in run_command(*args, **kwargs)
    403     ps = start_command(*args, **kwargs)
    404     returncode = ps.wait()
--> 405     return handle_errors(returncode, returncode, args, kwargs)
    406 
    407 

/home/pietro/docdat/src/gis/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/script/core.py in handle_errors(returncode, result, args, kwargs)
    321         args = make_command(*args, **kwargs)
    322         raise CalledModuleError(module=None, code=repr(args),
--> 323                                 returncode=returncode)
    324 
    325 def start_command(prog, flags="", overwrite=False, quiet=False,

CalledModuleError: Module run None ['g.region', '-p', "raster=b'elevation'"] ended with error
Process ended with non-zero return code 1. See errors in the (error) output.

In [4]: "%s=%s" % (b'raster', b'elevation')
Out[4]: "b'raster'=b'elevation'"

In [5]: "%s=%s" % (b'raster'.decode(), b'elevation'.decode())
Out[5]: 'raster=elevation'

Do you have an idea on how we could/should fix this?

[glynn]

Replying to zarch:

In [5]: "%s=%s" % (b'raster'.decode(), b'elevation'.decode())
Out[5]: 'raster=elevation'

Do you have an idea on how we could/should fix this?

Just avoid using string formatting for such trivial cases, e.g.:

args.append(opt.encode('ascii') + b'=' + _make_val(val)

To be honest, converting grass.script to Python 3 isn't going to be much fun, as a scripting library fundamentally revolves around dealing with byte strings (command-line arguments, environment variables, stdin/stdout), while Python 3 tries to pretend that byte strings are some kind of low-level implementation detail in a world where everything is Unicode.

[zarch]

Replying to glynn:

args.append(opt.encode('ascii') + b'=' + _make_val(val)

To be honest, converting grass.script to Python 3 isn't going to be much fun, as a scripting library fundamentally revolves around dealing with byte strings (command-line arguments, environment variables, stdin/stdout), while Python 3 tries to pretend that byte strings are some kind of low-level implementation detail in a world where everything is Unicode.

ok, I've followed your approach, now the make_command it is working only with bytes and return a list of bytes. Do you think that I could commit these changes in trunk? Other things that should be change before?

I've also started fixing several GRASS scripts to be parsed also with python3.

However I don't know how to fix the ctypes binding of GRASS, at the moment I get:

In [1]: from grass.lib import gis
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-1-9c20dc5e4f67> in <module>()
----> 1 from grass.lib import gis

/home/pietro/docdat/src/gis/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/lib/gis.py in <module>()
     13 _libdirs = []
     14 
---> 15 from ctypes_preamble import *
     16 from ctypes_preamble import _variadic_function
     17 from ctypes_loader import *

ImportError: No module named 'ctypes_preamble'

the problem here is that the gis.py file should start with:

from __future__ import absolute_import

...

from .ctypes_preamble import *
from .ctypes_preamble import _variadic_function
from .ctypes_loader import *

But these files are generated with make and I don't understand where should I change the code. Any hint?

[zarch]

reviewed version, use bytes instead of string (unicode)

[glynn]

Replying to zarch:

Do you think that I could commit these changes in trunk?

What's the oldest Python 2.x version they will work with?

Support for 2.7 is essential. We've been trying to maintain support for 2.6, but that may have to change in order to support 3.x.

However I don't know how to fix the ctypes binding of GRASS, at the moment I get:

But these files are generated with make and I don't understand where should I change the code. Any hint?

The lib/python/ctypes/fix.sed script inserts those imports.

If the __future__ import needs to be at the very start of the file, add the command

1i \
from __future__ import absolute_import

to the script.

If it just needs to be before any imports, add it to the start of the existing imports in the script.

[zarch]

Replying to glynn:

Replying to zarch:

Do you think that I could commit these changes in trunk?

What's the oldest Python 2.x version they will work with?

I did some test with:

• Python 2.6.9
• Python 2.7.10
• Python 3.4.3

Support for 2.7 is essential. We've been trying to maintain support for 2.6, but that may have to change in order to support 3.x.

I thjink should be possible to write code that it is compatible with 2.6/2.7 and 3.4. I will test for few more days and then I will commit in trunk if there are no objections

But these files are generated with make and I don't understand where should I change the code. Any hint?

The lib/python/ctypes/fix.sed script inserts those imports.

Thank you it works!

Now I get:

In [1]: from grass.lib import gis
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-9c20dc5e4f67> in <module>()
----> 1 from grass.lib import gis

/home/pietro/docdat/src/gis/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/lib/gis.py in <module>()
    800 ]
    801 struct_anon_11._fields_ = [
--> 802     ('__val', c_ulong * (1024 / (8 * sizeof(c_ulong)))),
    803 ]
    804 

TypeError: can't multiply sequence by non-int of type 'float'

Testing with the python debugger:

ipdb> (1024 / (8 * sizeof(c_ulong)))
16.0
ipdb> c_ulong * (1024 / (8 * sizeof(c_ulong)))
*** TypeError: can't multiply sequence by non-int of type 'float'

Do you have an idea on how we can fix this?

[glynn]

Replying to zarch:

Testing with the python debugger:

ipdb> (1024 / (8 * sizeof(c_ulong)))
16.0
ipdb> c_ulong * (1024 / (8 * sizeof(c_ulong)))
*** TypeError: can't multiply sequence by non-int of type 'float'

Do you have an idea on how we can fix this?

One option is to make ctypesgen use truncating division:

lib/python/ctypes/ctypesgencore/parser/cgrammar.py:282

-    '/': ("division", (lambda x,y: x/y), "(%s / %s)"),
+    '/': ("division", (lambda x,y: x/y), "(%s // %s)"),

However, this would break macros which perform floating-point division.

Another option would be to explicitly convert array sizes to integers, but that still doesn't handle the situation where a macro is expecting "/" to match C's division semantics (truncating division for integers, non-truncating division for floating-point values).

Ultimately, ctypesgen just translates macros directly to Python, so whichever division operator is used would be wrong for one case or the other. We could change it to use e.g. c_division(a,b) and define that function in ctypes_preamble.py. But that seems like overkill given that (on Linux) there are two occurrences of the division operator in the generated files.

One of them is for the definition of sigset_t, which is required for jmp_buf which in turn is required for G_fatal_longjmp(), and I'm not sure it's even possible to make use of that from Python. The other is for a macro named FUDGE() in ogsf.h, which I suspect is probably not particularly useful (and that one happens to be a floating-point division).

An alternative option is to just guard the <setjmp.h> include and the G_fatal_longjmp() declaration in defs/gis.h with #ifndef CTYPESGEN, and do likewise for the FUDGE() macro in ogsf.h.

However, that ignores the possibility that other platforms may have macros involving division in their system headers, or that future changes to GRASS may pull in additional headers with such macros.

Yet another option is a combination of the other two: prevent ctypesgen from ever seeing a macro involving division, and just remove the division case from mult_ops_dict so that if it does encounter one it raises an exception. That may require ongoing maintenance but avoids the situation where we end up silently generating broken conversions of macros.

[neteler]

Ticket retargeted after 7.0.1 milestone closed

[neteler]

Ticket retargeted after milestone closed

[neteler]

Ticket retargeted after milestone closed

[neteler]

Ticket retargeted after 7.0.3 milestone closed

[zarch]

Dear Glynn,

Replying to glynn:

Replying to zarch:

Testing with the python debugger:

ipdb> (1024 / (8 * sizeof(c_ulong)))
16.0
ipdb> c_ulong * (1024 / (8 * sizeof(c_ulong)))
*** TypeError: can't multiply sequence by non-int of type 'float'

Do you have an idea on how we can fix this?

One option is to make ctypesgen use truncating division:

lib/python/ctypes/ctypesgencore/parser/cgrammar.py:282

-    '/': ("division", (lambda x,y: x/y), "(%s / %s)"),
+    '/': ("division", (lambda x,y: x/y), "(%s // %s)"),

However, this would break macros which perform floating-point division.

Another option would be to explicitly convert array sizes to integers, but that still doesn't handle the situation where a macro is expecting "/" to match C's division semantics (truncating division for integers, non-truncating division for floating-point values).

Ultimately, ctypesgen just translates macros directly to Python, so whichever division operator is used would be wrong for one case or the other. We could change it to use e.g. c_division(a,b) and define that function in ctypes_preamble.py. But that seems like overkill given that (on Linux) there are two occurrences of the division operator in the generated files.

One of them is for the definition of sigset_t, which is required for jmp_buf which in turn is required for G_fatal_longjmp(), and I'm not sure it's even possible to make use of that from Python. The other is for a macro named FUDGE() in ogsf.h, which I suspect is probably not particularly useful (and that one happens to be a floating-point division).

An alternative option is to just guard the <setjmp.h> include and the G_fatal_longjmp() declaration in defs/gis.h with #ifndef CTYPESGEN, and do likewise for the FUDGE() macro in ogsf.h.

However, that ignores the possibility that other platforms may have macros involving division in their system headers, or that future changes to GRASS may pull in additional headers with such macros.

Yet another option is a combination of the other two: prevent ctypesgen from ever seeing a macro involving division, and just remove the division case from mult_ops_dict so that if it does encounter one it raises an exception. That may require ongoing maintenance but avoids the situation where we end up silently generating broken conversions of macros.

I was thinking to another option:

Can we modify fix.sed, to not only fix the import but also substitute the line:

    ('__val', c_ulong * (1024 / (8 * sizeof(c_ulong)))),

into:

    ('__val', c_ulong * (1024 // (8 * sizeof(c_ulong)))),

What do you think?

[glynn]

Replying to zarch:

Can we modify fix.sed, to not only fix the import but also substitute the line:

Matching the specific line would only work for glibc, and would break if the definition of __sigset_t ever changed in any way.

None of the other solutions are hard to implement; we just need to choose one.

If you just want to get this working now, I'd suggest using the "#ifndef CTYPESGEN" approach.

[neteler]

Is there a wiki or trac document how to perform python3 testing?

[martinl]

Milestone renamed

[neteler]

Replying to neteler:

Is there a wiki or trac document how to perform python3 testing?

ping ... still needed. Anyone?

[neteler]

See also #3446

[wenzeslaus]

In 71832:

init: use spaces consistently (no mixed tabs), required in Python 3 (PEP8 E101, W191, see #2708)

[wenzeslaus]

One option is to use Python virtual environments on your computer with Python 3 (I had to install python3-venv):

python3 -m venv test-python-3
source test-python-3/bin/activate

After r71832 and r71833 (already compiled) GRASS GIS is now able to start in text mode with Python 3 and give acceptable error messages for GUI.

[wenzeslaus]

In 71838:

fix most obvious Python 3 compatibility issues: tabs, has_key, print in po script (see #2708)

[wenzeslaus]

In 71849:

doc: Python 3 compatibility (see #2708)

Decode file content on input in 3, leave as in in 2.
Call constructor of parent class (Python 3 version needs to init a var).
Call less efficient items() in Python 2 (same code in both, although slower in 2, ok here).
This avoids compile (mkhtml) errors in most of the directories with Python 3.5.2 as python.

[wenzeslaus]

In 71852:

wxGUI: replace print statement by print function for Python 3 support, remove mixed tabs (PEP8 E101, W191, see #2708)

[wenzeslaus]

After r71838, r71839, r71840, r71838, and r71852 GRASS now compiles with Python 3 except for these three directories:

lib/python/ctypes
locale
man

Additionally, gui/wxpython also gives errors when you examine the directory itself.

[neteler]

Ticket retargeted after milestone closed

[neteler]

Replying to neteler:

Replying to neteler:

Is there a wiki or trac document how to perform python3 testing?

ping ... still needed. Anyone?

Now available at:

​https://trac.osgeo.org/grass/wiki/Python3Support#Howtotest

[martinl]

Original issue seems to be solved in trunk. Feel free to reopen if needed.