Opened 9 years ago
Closed 6 years ago
#2708 closed defect (fixed)
Run GRASS with Python3
Reported by: | zarch | Owned by: | |
---|---|---|---|
Priority: | normal | Milestone: | 7.8.0 |
Component: | Default | Version: | unspecified |
Keywords: | python2, python3 | Cc: | |
CPU: | Unspecified | Platform: | Unspecified |
Description
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?
Attachments (4)
Change History (34)
by , 9 years ago
Attachment: | init_grass.diff added |
---|
by , 9 years ago
Attachment: | gunittest.diff added |
---|
by , 9 years ago
Attachment: | script.diff added |
---|
follow-up: 2 comment:1 by , 9 years ago
follow-up: 3 comment:2 by , 9 years ago
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?
follow-up: 4 comment:3 by , 9 years ago
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.
follow-up: 5 comment:4 by , 9 years ago
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?
by , 9 years ago
Attachment: | script.2.diff added |
---|
reviewed version, use bytes instead of string (unicode)
follow-up: 6 comment:5 by , 9 years ago
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.
follow-up: 7 comment:6 by , 9 years ago
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?
follow-up: 12 comment:7 by , 9 years ago
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.
follow-up: 13 comment:12 by , 8 years ago
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?
comment:13 by , 8 years ago
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.
comment:14 by , 8 years ago
Milestone: | 7.0.4 → 7.0.5 |
---|
follow-up: 18 comment:15 by , 8 years ago
Is there a wiki or trac document how to perform python3 testing?
comment:16 by , 8 years ago
Milestone: | 7.0.5 → 7.3.0 |
---|
follow-up: 29 comment:18 by , 7 years ago
Replying to neteler:
Is there a wiki or trac document how to perform python3 testing?
ping ... still needed. Anyone?
comment:21 by , 7 years ago
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.
comment:25 by , 7 years ago
comment:26 by , 7 years ago
Keywords: | python2 python3 added |
---|
comment:28 by , 6 years ago
Milestone: | 7.4.1 → 7.4.2 |
---|
comment:29 by , 6 years ago
comment:30 by , 6 years ago
Milestone: | 7.4.2 → 7.8.0 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Original issue seems to be solved in trunk. Feel free to reopen if needed.
Replying to zarch:
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.