[GRASS-dev] [GRASS GIS] #3790: Cleanup gettext usage for python code

#3790: Cleanup gettext usage for python code
-------------------------+-------------------------
Reporter: pmav99 | Owner: grass-dev@…
     Type: enhancement | Status: new
Priority: normal | Milestone:
Component: Python | Version: svn-trunk
Keywords: | CPU: Unspecified
Platform: Unspecified |
-------------------------+-------------------------
GRASS uses 3 translation domains:
- grasslibs
- grassmods
- grasswxpy

Calling {{{gettext.install()}}} injects {{{_()}}} in the builtins
namespace thus making it globally available. To make i18n work, we should
only need to call {{{gettext.install()}}} once per translation domain.

The provided patch tries to simplify and centralize the usage of the
gettext module in the python code. The main idea is that
{{{lib/python/__init__.py}}} should be the one and only place where
{{{getetx.install()}}} calls are being made. The direct consequence of
this is that any python code that needs to use {{{_()}}} only needs to
import the {{{grass}}} library first. This includes both the GUI code and
the "scripts".

The patch is attached and can be applied with:
{{{
patch -p0 < gettext_cleanup.diff
}}}

The changes can also be browsed [https://github.com/GRASS-GIS/grass-
ci/pull/9 here].

In order to test this you need to:

1. Enable a non-english language locale on your OS. The exact way you do
that depends on your distribution. French and German are probably the best
options since AFAIK they have the most complete translations, but any
supported language should work.
2. Configure GRASS with NLS {{{--with-nls}}} and re-compile. If NLS was
not previously enabled you need to run {{{make distclean}}} before
configuring/compiling.
3. Start a GRASS session with {{{LC_ALL=fr_FR.utf-8 ./bin.x86_64-pc-linux-
gnu/grass77}}} (note you don't need to export any ENV variables; LC_ALL's
purpose is specifically to override the values of all the other LC_*
variables.[https://wiki.archlinux.org/index.php/Locale#LC_ALL:_troubleshooting
source])
4. your new session should now be in French. You should see the "text-
based splash screen" in French. The command descriptions/options in french
+ the GUI in french. If not, that's a bug (or a missing translation!).

What has not been tested is the doctests. I would appreciate if someone
could verify that they work and/or provide info on how to run them.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: new
  Priority: normal | Milestone:
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Changes (by pmav99):

* Attachment "gettext_cleanup.diff" added.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790&gt;
GRASS GIS <https://grass.osgeo.org>

Hi devs,

GRASS GIS <trac@osgeo.org> schrieb am Di., 12. März 2019, 08:45:

#3790: Cleanup gettext usage for python code
--------------------------±------------------------
Reporter: pmav99 | Owner: grass-dev@…
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: Python | Version: svn-trunk
Resolution: | Keywords:
CPU: Unspecified | Platform: Unspecified
--------------------------±------------------------
Changes (by pmav99):

  • Attachment “gettext_cleanup.diff” added.

Are there any objections to apply the changes proposed by pmav99?

Thanks,
Markus


Ticket URL: <https://trac.osgeo.org/grass/ticket/3790>
GRASS GIS <https://grass.osgeo.org>


grass-dev mailing list
grass-dev@lists.osgeo.org
https://lists.osgeo.org/mailman/listinfo/grass-dev

On Sat, Mar 16, 2019 at 3:32 PM Markus Neteler <neteler@osgeo.org> wrote:

Hi devs,

GRASS GIS <trac@osgeo.org> schrieb am Di., 12. März 2019, 08:45:

#3790: Cleanup gettext usage for python code
--------------------------±------------------------
Reporter: pmav99 | Owner: grass-dev@…
Type: enhancement | Status: new
Priority: normal | Milestone:
Component: Python | Version: svn-trunk
Resolution: | Keywords:
CPU: Unspecified | Platform: Unspecified
--------------------------±------------------------
Changes (by pmav99):

  • Attachment “gettext_cleanup.diff” added.

Are there any objections to apply the changes proposed by pmav99?

The patch looks good to me, it provides code simplification and the location for the import of gettext makes sense.

Markus M

Thanks,
Markus


Ticket URL: <https://trac.osgeo.org/grass/ticket/3790>
GRASS GIS <https://grass.osgeo.org>


grass-dev mailing list
grass-dev@lists.osgeo.org
https://lists.osgeo.org/mailman/listinfo/grass-dev


grass-dev mailing list
grass-dev@lists.osgeo.org
https://lists.osgeo.org/mailman/listinfo/grass-dev

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: new
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Changes (by martinl):

* milestone: => 7.8.0

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:1&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: new
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Changes (by pmav99):

* Attachment "gettext_cleanup2.diff" added.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: new
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by pmav99):

There were some conflicts after [74264]. I uploaded a new patch.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:2&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: closed
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: fixed | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Changes (by neteler):

* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"74307" 74307]:
{{{
#!CommitTicketReference repository="" revision="74307"
i18N: cleanup gettext usage for Python code (fixes #3790) (contributed by
pmav99)
}}}

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:3&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: closed
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: fixed | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by wenzeslaus):

The things in lib/python are potentially (and actually) used as Python
modules by other applications or scripts. Thus from
https://pymotw.com/2/gettext/index.html#module-localization, it is
actually the second option which applies:

> //For a library, or individual module, modifying `__builtins__` is not a
good idea because you don’t know what conflicts you might introduce with
an application global value. You can import or re-bind the names of
translation functions by hand at the top of your module.//

The doctests and interative command line are probably where the issues
will happen first. See Django practices from
https://docs.djangoproject.com/en/dev/topics/i18n/translation/ as linked
in #1739:

> //Python’s standard library gettext module installs _() into the global
namespace, as an alias for gettext(). In Django, we have chosen not to
follow this practice, for a couple of reasons: //
> //Sometimes, you should use gettext_lazy() as the default translation
method for a particular file. Without _() in the global namespace, the
developer has to think about which is the most appropriate translation
function.//

> //The underscore character (_) is used to represent “the previous
result” in Python’s interactive shell and doctest tests. Installing a
global _() function causes interference. Explicitly importing gettext() as
_() avoids this problem.//

To run doctests, use

{{{
python -m doctest -v example.py
}}}

or (often already included in code):

{{{
if __name__ == "__main__":
     import doctest
     doctest.testmod()
}}}

both taken from https://docs.python.org/2/library/doctest.html, so the
question is if you have an particular issue. Another things to test are
Python in command line, Python console in GUI, and Jupyter Notebook. (But
the doctest are where the issues definitively emerged in the past.)

Please note that I never understood all the details of #1739 as noted
there, so maybe we have things to investigate more. Also, try to examine
`do_doctest_gettext_workaround()` which doctests need when the builin is
modified or perhaps even in other cases.

Finally, is there any overhead in centralizing the tr string installation?
So far we were relatively successful in keeping the GUI separate (whether
for performance or code organization reasons).

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:4&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: closed
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: fixed | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by pmav99):

Disclaimer: The real goal of this refactoring was to get rid of
{{{os.getenv("GISBASE")}}} calls at the module level. This is a
prerequisite for #3772. The scripts and the GUI code didn't really need to
be touched, but I did them too for uniformity.

Replying to [comment:4 wenzeslaus]:
> The things in lib/python are potentially (and actually) used as Python
modules by other applications or scripts. Thus from
https://pymotw.com/2/gettext/index.html#module-localization, it is
actually the second option which applies:
>
> > //For a library, or individual module, modifying `__builtins__` is not
a good idea because you don’t know what conflicts you might introduce with
an application global value. You can import or re-bind the names of
translation functions by hand at the top of your module.//

I really like it when things are explicit. I dislike the idea of injecting
things into builtins and I do found the convention of using {{{_()}}} as
an alias for {{{gettext.gettext()}}} rather unfortunate. Consequently, I
don't have any objections with getting rid of {{{gettext.install()}}}
and/or even {{{_()}}}.

Regardless, I am not sure that GRASS was actually following Doug Helman's
advice. More specifically, almost all GRASS modules used to import the
{{{grass}}} library before actually calling {{{gettext.install()}}} on
their own ([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/0983ca068d6f7363d2fede26f77b8aed76c87bcc source]). The
{{{grass}}} library was already calling {{{gettext.install}}}
([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/b589754aa497ac8ad067ed8258b12caaa0bf1e17#diff-
15e2e35efef103adc7f77c0a3afe4a63 source]). {{{grass.temporal}}} was doing
the same for no apparent reason though, since it was just re-registering
the "grasslibs" domain ([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/b589754aa497ac8ad067ed8258b12caaa0bf1e17#diff-
e36b6b10b67c75c0d7c159b273854ef1 source]). Anyway, the end result is that
most of the time, if not always, as soon as you imported the {{{grass}}}
lib, the {{{builtins}}} were modified anyway.

In other words, I don't think that the proposed patch really changes what
the {{{grass}}} library was doing. The builtin injection continues to
happen just like it used before.

That being said, I am completely open to any improvements. I am also
perfectly fine with following Mr. Helman's advice (i.e. by adding explicit
imports in each and every {{{grass}}} lib module) if someone wants to
contribute that. But IMHV the proposed implementation is cleaner than the
previous one.

> So far we were relatively successful in keeping the GUI separate
(whether for performance or code organization reasons).

I don't really see any coupling of the GUI code with the {{{grass}}}
library. All the {{{grass}}} library does is registering the "grasswxpy"
domain. It doesn't import anything.

TBH, I did consider moving the {{{gettext.install("grasswxpy", ...)}}}
code inside {{{./gui/wxpython}}}, but the way it is structured doesn't
allow to do that cleanly and I didn't want to make changes there too. It
is doable though.

> Finally, is there any overhead in centralizing the tr string
installation?

I haven't measured it. Obviously there is some overhead. Previously when
you were importing {{{grass}}} for the first time in a process, you were
registering 2 domains. "grasslibs" + "grassmods" when using the CLI and
"grasslibs" + "grasswxpy" when using the GUI. Now 3 domains are being
registered. So there is overhead.

TBH I haven't read the whole {{{gettext}}} module line by line, but I
think that the gist of {{{gettext.install()}}} is
[https://github.com/python/cpython/blob/ead15795986972690217e52087eb946b8a5bbcda/Lib/gettext.py#L512-L552
these lines]. Line 552 calls the method that does the builtin injection
(check lines 314-316) while line 551 calls the function that registers the
domain in a "private" dictionary named {{{_translations}}}. AFAI can tell
from just reading the code, the only overhead to really speak of is the
parsing of the {{{*.mo}}} files which in GRASS are a few hundred KBs. On
SSDs this overhead should be negligible while on mechanical drives the
bottleneck is not going to be the code anyway. Re-registering a domain
should incur any overhead since the results are cached.

That being said, if you feel that it is necessary, it should be possible
to avoid registering "grasswxpy" and "grassmods" inside grass lib. E.g. by
adding a couple of functions in {{{./lib/python/__init__.py}}}:
{{{

# ...
_LOCALE_DIR = os.path.join(os.getenv("GISBASE"), 'locale')
gettext.install('grasslibs', _LOCALE_DIR)

def register_grassmods():
     gettext.install('grassmods', _LOCALE_DIR)

def register_grasswxpy():
     gettext.install('grasswxpy', _LOCALE_DIR)

}}}

and making sure that they are imported by each and every module that needs
them. But I am curious if the impact is that significant to make it worth
it.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:5&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: reopened
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------
Changes (by neteler):

* status: closed => reopened
* resolution: fixed =>

Comment:

Replying to [comment:3 neteler]:
> In [changeset:"74307" 74307]:
> {{{
> #!CommitTicketReference repository="" revision="74307"
> i18N: cleanup gettext usage for Python code (fixes #3790) (contributed
by pmav99)
> }}}

According to #3838 now wxGUI is broken... reopening.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:6&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: reopened
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by neteler):

Here, how to see the (or one of the) errors easily:

{{{
GRASS 7.7.svn (nc_spm_08_grass7):~ > d.mon wx0
GRASS 7.7.svn (nc_spm_08_grass7):~ > Traceback (most recent call last):
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/mapdisp/main.py", line 47, in <module>
     from core.render import Map, MapLayer, Overlay, RenderMapMgr
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/render.py", line 42, in <module>
     from core.ws import RenderWMSMgr
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/ws.py", line 33, in <module>
     from core.gthread import gThread
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/gthread.py", line 28, in <module>
     from core.gconsole import EVT_CMD_DONE, wxCmdDone
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/gconsole.py", line 49, in <module>
     from gui_core.forms import GUI
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/gui_core/forms.py", line 98, in <module>
     from gui_core.widgets import StaticWrapText, ScrolledPanel,
ColorTablesComboBox, \
   File "/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/gui_core/widgets.py", line 90, in <module>
     from core.utils import _
ImportError: cannot import name '_' from 'core.utils'
(/home/mneteler/software/grass77/dist.x86_64-pc-linux-
gnu/gui/wxpython/core/utils.py)

}}}

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:7&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: reopened
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by pmav99):

Are you sure this is at trunk Markus? Since 74307
{{{gui_core/widgets.py}}} shouldn't be trying to import {{{_}}}.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:8&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: reopened
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by neteler):

Replying to [comment:8 pmav99]:
> Are you sure this is at trunk Markus? Since 74307
{{{gui_core/widgets.py}}} shouldn't be trying to import {{{_}}}.

Oh, apparently a old compilation leftover. After "make distclean" and
recompilation it works (right now using Python 2). Sorry for the "d.mon"
noise.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:9&gt;
GRASS GIS <https://grass.osgeo.org>

#3790: Cleanup gettext usage for python code
--------------------------+-------------------------
  Reporter: pmav99 | Owner: grass-dev@…
      Type: enhancement | Status: reopened
  Priority: normal | Milestone: 7.8.0
Component: Python | Version: svn-trunk
Resolution: | Keywords:
       CPU: Unspecified | Platform: Unspecified
--------------------------+-------------------------

Comment (by wenzeslaus):

Replying to [comment:5 pmav99]:
> Disclaimer: The real goal of this refactoring was to get rid of
{{{os.getenv("GISBASE")}}} calls at the module level from all over the
place. This is a prerequisite for #3772. The scripts and the GUI code
didn't really need to be touched, but I did them too for uniformity.

You will have to help me here. Why is the relative path solution from
#3772 not applicable to the explicit import approach?

In relation to that, since #3772 is about using `grass` as a standard
Python package, I think it should follow the "Helman's advice" for a
Python package.

> Replying to [comment:4 wenzeslaus]:
> > The things in lib/python are potentially (and actually) used as Python
modules by other applications or scripts. Thus from
https://pymotw.com/2/gettext/index.html#module-localization, it is
actually the second option which applies:
> >
> > > //For a library, or individual module, modifying `__builtins__` is
not a good idea because you don’t know what conflicts you might introduce
with an application global value. You can import or re-bind the names of
translation functions by hand at the top of your module.//
>
> ...
>
> I am not sure that GRASS was actually following Doug Helman's advice.
More specifically, almost all GRASS modules used to import the {{{grass}}}
library before actually calling {{{gettext.install()}}} on their own
([https://github.com/GRASS-GIS/grass-
ci/pull/9/commits/0983ca068d6f7363d2fede26f77b8aed76c87bcc source]). ...
Anyway, the end result is that most of the time, if not always, as soon as
you imported the {{{grass}}} lib, the {{{builtins}}} were modified anyway.

Please read #1739 and #2425 linked from there. I explain that the wxGUI is
using the practice while the library (lib/python) is not and it should.

> That being said, I am completely open to any improvements. I am also
perfectly fine with following Mr. Helman's advice (i.e. by adding explicit
imports in each and every {{{grass}}} lib module) if someone wants to
contribute that. But IMHV the proposed implementation is cleaner than the
previous one.

Cleaner in which way? I think the registration of the domains separately
and all at once is a separate issue from "Helman's advice" (which is
notably also the way which Django is following).

> > So far we were relatively successful in keeping the GUI separate
(whether for performance or code organization reasons).
>
> I don't really see any coupling of the GUI code with the {{{grass}}}
library. All the {{{grass}}} library does is registering the "grasswxpy"
domain. It doesn't import anything.

It is not just the imports. The idea is that you should be able to install
GRASS GIS as command line only application without any GUI parts. But
there is more than one way to deal with this. If the registration of the
domain can just fail silently, that's probably good enough in this
particular case. However, as I said, this is a separate issue.

What is crucial here are the explicit imports versus using builtin. There
were issues with builtin solution in the past (hence #1739), builtin `_`
currently causes issues (doctests, static code linting), builtin solution
is not considered good practice for libraries (Helman, Django) and the
change breaks wxGUI according to #3838. Thus, I suggest reverting it (at
least the builtin part) and finding a different solution for #3772.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/3790#comment:10&gt;
GRASS GIS <https://grass.osgeo.org>