[GRASS-dev] PyGRASS Module error handling

Hi,

when writing tests which involve PyGRASS Module class I found that at some occasions the error messages are quite confusing.

First when you try to run one module twice, you get AttributeError:

from grass.pygrass.modules import Module
from subprocess import PIPE
m = Module(‘r.info’, map=‘elevation’, run_=False, stdout_=PIPE)
m.run()
m.outputs[‘stdout’].value

running for the second time

m.run()
Traceback (most recent call last)

AttributeError: ‘str’ object has no attribute ‘fileno’

Second, when you confuse m.inputs[‘map’].value with m.inputs[‘map’] (which is pretty easy error to do) you get “TypeError: deletions are implemented differently for unicode”:

this is OK

m.inputs[‘map’].value = 5
Traceback (most recent call last)

TypeError: The Parameter , require: raster, get: <type ‘int’> instead

this is not OK

confusing parameter object and parameter value

m.inputs[‘map’] = ‘aspect’
Traceback (most recent call last)

TypeError: deletions are implemented differently for unicode

With the former it depends what is actually allowed. Than it is probably easy to fix. With the later, no easy solutions come to my mind.

With my tests I was actually asserting m.outputs[‘stdout’] for true which is always true instead of asserting m.inputs[‘stdout’].value. I don’t know how to avoid this mistake in general.

Does this make sense to you or do you think that I and PyGRASS users should be just be more careful?

Vaclav

Hi Vaclav,

2014-06-30 17:43 GMT+02:00 Vaclav Petras <wenzeslaus@gmail.com>:

Hi,

when writing tests which involve PyGRASS Module class I found that at some
occasions the error messages are quite confusing.

First when you try to run one module twice, you get AttributeError:

from grass.pygrass.modules import Module
from subprocess import PIPE
m = Module('r.info', map='elevation', run_=False, stdout_=PIPE)
m.run()
m.outputs['stdout'].value

...

# running for the second time
m.run()

Traceback (most recent call last)
    ...
AttributeError: 'str' object has no attribute 'fileno'

That should be fixed in r61083. Reason was the wrong handling of
stderr and stdout ,
the PIPE was overwritten when the module finished.

Second, when you confuse m.inputs['map'].value with m.inputs['map'] (which
is pretty easy error to do) you get "TypeError: deletions are implemented
differently for unicode":

# this is OK
m.inputs['map'].value = 5

Traceback (most recent call last)
    ...
TypeError: The Parameter <map>, require: raster, get: <type 'int'> instead

# this is not OK
# confusing parameter object and parameter value
m.inputs['map'] = 'aspect'

Traceback (most recent call last)
    ...
TypeError: deletions are implemented differently for unicode

This is indeed a misleading error report. I do not have a solution right away,
but i would suggest to use always the __call__ function when
specifying an option:

color = Module("r.colors", run_=False)
color(map="test")
color(color="gyr")
color.run()
color(map="test2", color="ryb")
color.run()

With the former it depends what is actually allowed. Than it is probably
easy to fix. With the later, no easy solutions come to my mind.

With my tests I was actually asserting m.outputs['stdout'] for true which is
always true instead of asserting m.inputs['stdout'].value. I don't know how
to avoid this mistake in general.

We should avoid in general to set the option values directly, instead
the __call__ function should be used
which is much safer.

Does this make sense to you or do you think that I and PyGRASS users should
be just be more careful?

The error report must be modified to be more meaningful. I would
suggest to overwrite the assignment function "=" (if this is
possible???) of the Parameter class to catch such errors. The
assignment method will work in case of a Parameter object and raise a
meaningful error in case anything else. Does this sounds reasonable?

Best regards
Soeren

Vaclav

On Mon, Jun 30, 2014 at 8:33 PM, Sören Gebbert <soerengebbert@googlemail.com

wrote:

Hi Vaclav,

2014-06-30 17:43 GMT+02:00 Vaclav Petras <wenzeslaus@gmail.com>:
> Hi,
>
> when writing tests which involve PyGRASS Module class I found that at
some
> occasions the error messages are quite confusing.
>
> First when you try to run one module twice, you get AttributeError:
>
>>>> from grass.pygrass.modules import Module
>>>> from subprocess import PIPE
>>>> m = Module('r.info', map='elevation', run_=False, stdout_=PIPE)
>>>> m.run()
>>>> m.outputs['stdout'].value
> ...
>>>> # running for the second time
>>>> m.run()
> Traceback (most recent call last)
> ...
> AttributeError: 'str' object has no attribute 'fileno'

That should be fixed in r61083. Reason was the wrong handling of
stderr and stdout ,
the PIPE was overwritten when the module finished.

Thanks.

>
> Second, when you confuse m.inputs['map'].value with m.inputs['map']
(which
> is pretty easy error to do) you get "TypeError: deletions are implemented
> differently for unicode":
>
>>>> # this is OK
>>>> m.inputs['map'].value = 5
> Traceback (most recent call last)
> ...
> TypeError: The Parameter <map>, require: raster, get: <type 'int'>
instead
>>>> # this is not OK
>>>> # confusing parameter object and parameter value
>>>> m.inputs['map'] = 'aspect'
> Traceback (most recent call last)
> ...
> TypeError: deletions are implemented differently for unicode

This is indeed a misleading error report. I do not have a solution right
away,
but i would suggest to use always the __call__ function when
specifying an option:

color = Module("r.colors", run_=False)
color(map="test")
color(color="gyr")
color.run()
color(map="test2", color="ryb")
color.run()

I must get used to this one.

> With the former it depends what is actually allowed. Than it is probably
> easy to fix. With the later, no easy solutions come to my mind.
>
> With my tests I was actually asserting m.outputs['stdout'] for true
which is
> always true instead of asserting m.inputs['stdout'].value. I don't know
how
> to avoid this mistake in general.

We should avoid in general to set the option values directly, instead
the __call__ function should be used
which is much safer.

> Does this make sense to you or do you think that I and PyGRASS users
should
> be just be more careful?

The error report must be modified to be more meaningful. I would
suggest to overwrite the assignment function "=" (if this is
possible???) of the Parameter class to catch such errors. The
assignment method will work in case of a Parameter object and raise a
meaningful error in case anything else. Does this sounds reasonable?

I must admit that I'm not sure whose assignment operator is called.

Parameters, or the function for adding item to the dictionary? Assignment
operator cannot be overloaded in Python. So, it seems that it must the the
later. So, the only solution is TypeDict which is already used. I'm
confused. Now focusing on something else.

Thanks,
Vaclav

Best regards
Soeren

>
> Vaclav

Hi folks,

sorry for the silence...

On Tue, Jul 1, 2014 at 2:33 AM, Sören Gebbert
<soerengebbert@googlemail.com> wrote:

2014-06-30 17:43 GMT+02:00 Vaclav Petras <wenzeslaus@gmail.com>:

# running for the second time
m.run()

Traceback (most recent call last)
    ...
AttributeError: 'str' object has no attribute 'fileno'

That should be fixed in r61083. Reason was the wrong handling of
stderr and stdout ,
the PIPE was overwritten when the module finished.

Thanks for the fix!

Second, when you confuse m.inputs['map'].value with m.inputs['map'] (which
is pretty easy error to do) you get "TypeError: deletions are implemented
differently for unicode":

# this is OK
m.inputs['map'].value = 5

Traceback (most recent call last)
    ...
TypeError: The Parameter <map>, require: raster, get: <type 'int'> instead

# this is not OK
# confusing parameter object and parameter value
m.inputs['map'] = 'aspect'

Traceback (most recent call last)
    ...
TypeError: deletions are implemented differently for unicode

Sorry for the misleading error. I try to explain a bit better what is
going on...

IMHO the best way to set/change the parameter value is:

m.inputs.map

'slope'

m.inputs.map = 'aspect'

Using the syntax:

m.inputs['map'].value

We are asking to a dictionary the object Parameter labelled 'map' and
therefore access to the attribute value of Parameter class.

When you are confusing the parameter object with the value:

m.inputs['map'] = 'aspect'

you are trying to overwrite the instance Parameter with a unicode
string, and there was an error, now should be clear (r61091):

from grass.pygrass.modules import Module
m = Module("r.slope.aspect")
m.inputs['elevation'] = "elevation"

Traceback (most recent call last):
  File "<ipython-input-4-047eba12e7ac>", line 1, in <module>
    m.inputs['elevation'] = "elevation"
  File "/home/pietro/docdat/src/gis/grass71/dist.x86_64-unknown-linux-gnu/etc/python/grass/pygrass/modules/interface/typedict.py",
line 40, in __setitem__
    raise TypeError(str_err % (value, self._type.__name__))
TypeError: The value: 'elevation' is not a Parameter instance.

This is indeed a misleading error report. I do not have a solution right away,
but i would suggest to use always the __call__ function when
specifying an option:

color = Module("r.colors", run_=False)
color(map="test")
color(color="gyr")
color.run()
color(map="test2", color="ryb")
color.run()

With the former it depends what is actually allowed. Than it is probably
easy to fix. With the later, no easy solutions come to my mind.

With my tests I was actually asserting m.outputs['stdout'] for true which is
always true instead of asserting m.inputs['stdout'].value. I don't know how
to avoid this mistake in general.

yes it is annoying! but using this syntax you are accessing at the
Parameter instance, this is why I add the other syntax.

IMHO are both useful, when I'm writing a GRASS script I prefer to
access through m.inputs.parameter_name instead when I writing code
that have to handle a Module instance it is useful to be able to
access as a dictionary with: m.inputs['parameter_name'].value

this is the best that I was able to do, to make it smoother and
easier, but If you have any better Idea, please tell me, I will happy
to change the current approach.

We should avoid in general to set the option values directly, instead
the __call__ function should be used
which is much safer.

Using the __call__ method these operations are done internally and you
are pretty sure to not confuse and/or overwrite the Parameter
instance.

Does this make sense to you or do you think that I and PyGRASS users should
be just be more careful?

The error report must be modified to be more meaningful. I would
suggest to overwrite the assignment function "=" (if this is
possible???) of the Parameter class to catch such errors. The
assignment method will work in case of a Parameter object and raise a
meaningful error in case anything else. Does this sounds reasonable?

Hi hope that the message error now is clearer, let me know if you
think that still need to be changed!

Pietro

On Tue, Jul 1, 2014 at 3:10 AM, Vaclav Petras <wenzeslaus@gmail.com> wrote:

I must admit that I'm not sure whose assignment operator is called.
Parameters, or the function for adding item to the dictionary? Assignment
operator cannot be overloaded in Python. So, it seems that it must the the
later. So, the only solution is TypeDict which is already used. I'm
confused. Now focusing on something else.

the attributes: inputs and outputs are TypeDict objects that inherit
from OrderedDict and just check before to insert in the dictionary
that the value is of a certain type, in our case a Parameter instance.
Se the method that is called when you are trying to set an item to a
TypeDict is __setitem__.

I hope it makes sense.

Best regards

Pietro