Define rational to handle exception

[gschwind]

Overview

This patch series propose to define clearly how to handle Exception to ensure they are correctly reported by a standard WPS.

The first patch redefine the process status to match the definition given by WPS standard. The second patch depend on it.

The second patch actually define how Exception should be handled, and what each functions must do to ensure the proper output of the services. It's also implement the rational.

Best regards

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

• I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
• I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

[gschwind]

To pass some unit test this patch series require #361 . I working on this patch series to pass units missing tests

Best regards

[gschwind]

I updated the branch to pass unit tests with #361

[cehbrecht]

unit tests pass ... but pep8 checks fail. Please update the files accordingly.

[cehbrecht]

@gschwind I have made a quick check with your PR but I'm running into an exception (post request). I'm using your patched pywps with my Emu WPS:
https://github.com/bird-house/emu

When I run an excecute post process (birdy, owslib) with the hello process I get the following exception:

2018-07-16 13:39:27,895] [ERROR] line=45 module=exceptions Exception: code: 500, locator: No applicable error code, please check error log, description: 
Traceback (most recent call last):
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Service.py", line 428, in call
    request_uuid
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Service.py", line 83, in execute
    return self._parse_and_execute(process, wps_request, uuid)
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Service.py", line 162, in _parse_and_execute
    wps_response = process.execute(wps_request, uuid)
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Process.py", line 118, in execute
    wps_response = self._execute_process(self.async, wps_request, wps_response)
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Process.py", line 157, in _execute_process
    self._run_async(wps_request, wps_response)
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Process.py", line 179, in _run_async
    wps_response.update_status(self.status, u"PyWPS Request accepted", 0)
AttributeError: 'SayHello' object has no attribute 'status'

I suppose there are still methods that need to be converted for the status handling.

[gschwind]

Thank you for the review I will have a closer look at it this evening (CET)

[coveralls]

Coverage increased (+0.3%) to 73.443% when pulling c6b7570 on gschwind:define-rational-to-handle-exception into abdd4a3 on geopython:master.

[coveralls]

Coverage decreased (-1.6%) to 72.458% when pulling 7c6c972 on gschwind:define-rational-to-handle-exception into 1306d4e on geopython:master.

[gschwind]

Should be fine now :)

Best regards

[cehbrecht]

Sorry ... I still need to complain. Tried it with the sleep Process of Emu:

Traceback (most recent call last):
  File "/Users/pingu/Documents/GitHub/birdhouse/pywps/pywps/app/Process.py", line 200, in _run_process                                              
    self.handler(wps_request, wps_response)  # the user must update the wps_response.                                                               
  File "/Users/pingu/Documents/GitHub/birdhouse/emu/emu/processes/wps_sleep.py", line 44, in _handler                                               
    response.update_status('PyWPS Process started. Waiting...', 20)
TypeError: update_status() missing 1 required positional argument: 'status_percentage'        

[gschwind]

Hello,

Do not be sorry for that, this is the point of review. Thanks for your review.

The patch break the API of update_status, I choose to break the API instead of keeping the old one.

I can write a code that somehow map the old API to the new one, but does it wanted ?

If it is wanted I can update the code to keep the old API.

in your case the line of the old API
response.update_status('PyWPS Process started. Waiting...', 20)
should be replaced by:

from pywps.response.status import WPS_STATUS
response.update_status(WPS_STATUS.STARTED, 'PyWPS Process started. Waiting...', 20)

Best regard

[cehbrecht]

does that mean the user defined process has to decide which status the process execution currently has? I think that should be handled by pywps internally .... the process code just gives progress information.

[gschwind]

Yes it was the idea, in particular to set it to paused or failed. It was my understanding because the user can change the status with the old API, using the status argument.

But maybe I'm wrong :)

[gschwind]

Changing it is trivial, so just tell me what is preferred

[cehbrecht]

I would prefer if the process can optionally set job status. But by default pywps should handle the states internally. @jachym should have the final word :)

[jachym]

Sorry for long time take me to review this (FOSS4G syndrome), but I would like to say, that this patch is very well needed, thank you @gschwind you dived into this.

I agree with @cehbrecht that the process itself should not set WPS_STATUS state internally, this should be done from PyWPS automagically - but the process should set status message (and percent done).

Reasoning:

• When the request arrives to the server, forst status is ProcessAccepted
• Then the calculation starts, therefore the status is set to ProcessStarted
• When the calculation is done, the process is set to ProcessSuccessfull (iirc)
• If something fails, its set to ProcessFailed

In the future ProcessPaused and other are going to be set from the main thread (from PyWPS) - the process should not inteact with this status.

Only two statuses, which could be considered, is that the process could explicetly state ProcessSuccessfull or ProcessFailed - but this is ensured either with standard return response or by any kind of exception.

Do you agree?

[cehbrecht]

Well, I agree. @jachym thanks for giving more insight on this. @gschwind do you like to make the changes?

[gschwind]

Hello,

@jachym: I'm fine with your explanation, with few comment,

I agree to limit the control of user over the response status, I that case I would remove the status from the public API.

About the life time of the request I would describe it as follow:

• When the request arrives to the server the status is Unknow until the request is validated, e.g. check for valid GetCapabilities, DescribeProcess or Execute request, if an error occur in that stage, the error is reported as XML WPSException.
• Then once validated the status is ProcessAccepted, from this stage, the errors are reported by a XML WPSException if the request is a RAW, and by the status file with WPSException otherwise.
• Just before the handler is called, the calculation starts, therefore the status is set to ProcessStarted. e.g. while the process is in queue in particular for async mode, it's status remains ProcessAccepted till it's actually started.
• When the calculation is done, the process is set to ProcessSuccessfull (iirc)
• If the handler raise an Exception, its set to ProcessFailed

In the current version, I expect that the process is successful if no exception is raised. I do not consider the return value of the handler, I want to avoid that the user instantiate it's own response by mistake.

@cehbrecht : Yes I will do this :), just wait for the week-end ;)

Best regards

[gschwind]

Hello,

updated according previous discussion :)

Best regards

[jachym]

nice

[jachym]

Thanks a lot @gschwind

I do not fully understand, what was your motivation to dive into this, but it was needed - you are my hero.

[gschwind]

Hello,

I use PyWPS to serve some of our WPS, having a better code base for pywps is helping for future improvement, like the support of WPS 2.0.0.

Basically it's the way of open souces/free software.

You are my hero to maintain this codes :)