Simplify the implementation of IOHandler #602

gschwind · 2021-04-25T07:53:18Z

Overview

The current implementation use a tricks that manipulate the mro.
This trick do not have any obvious benefit and make the code much more
complicated than necessary. This patch simplifier the implementation by
replacing the tricky class change to a change in mamber's variable.

Related Issue / Discussion

Following discussion in #598, Here is my proposal of refactoring the IOHandler. It come in two flavor, one with is a basic one and a second using weakref. I thought of another way using only class object without instantiating them like the previous implementation but I don't like too much the idea.

The code pass all the test excluding two. the two failed test a due to the fact that the test use private API to trick the test checking, thus IMO this just the test that is invalid.

Best regards.

Additional Information

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

huard

Made some proposals to fix failing tests here: gschwind#1

Otherwise this looks good and makes the code clearer I believe.

huard · 2021-04-26T17:45:30Z

pywps/inout/basic.py

+    @prop.setter
+    def prop(self, value):
+        LOGGER.warning("Deprecated use of IOHandler.prop, prefer self.value = self.value")
+        if value == "file":
+            self._iohandler = FileHandler(self._iohandler.file, self)
+        elif value == "data":
+            self._iohandler = DataHandler(self._iohandler.data, self)
+        elif value == "stream":
+            self._iohandler = StreamHandler(self._iohandler.stream, self)
+        elif value == "url":
+            self._iohandler = UrlHandler(self._iohandler.url, self)


Do we really need this ? Removing it does not create any test failure. I don't think this was supported previously.

Hello huard,

At beginning I did not implement the prop setter but the test : test_inout.py:test_complex_input_stream need it.

Otherwise, I agree that we should remove it.

huard · 2021-04-26T19:25:55Z

I've tested this with emu + birdy and I'm getting 3 INTERNAL SERVER ERRORS.

https://github.com/bird-house/emu
https://github.com/bird-house/birdy

To reproduce:

Install emu and launch it with emu start
Install birdy and run the test suite with pytest

gschwind · 2021-04-27T07:20:44Z

Hello huart,

I will have a look, best regards.

coveralls · 2021-04-27T08:30:55Z

Coverage remained the same at 0.0% when pulling babc8fc on gschwind:pywps-4.4-007 into 559f13b on geopython:pywps-4.4.

gschwind · 2021-04-27T21:10:31Z

Hello huard,

I found the guilty :)

Thank you for your feedbacks

Best regards

The current implementation use a tricks that manipulate the __mro__. This trick do not have any obvious benefit and make the code much more complicated than necessary. This patch simplifier the implementation by replacing the tricky class change to a change in mamber's variable.

cehbrecht · 2021-05-06T17:05:59Z

@gschwind @huard merged . Thanks :)

…ion (relates to geopython/pywps#526, caused by geopython/pywps@343d825, PR geopython/pywps#602)

…ation (relates to geopython/pywps#526, caused by geopython/pywps@343d825, PR geopython/pywps#602)

huard · 2022-05-12T20:51:55Z

Hi @gschwind

I'm working on parallelism in PyWPS and I'm hitting an issue with the serialization of Process instances. The IO handlers include weakrefs that are not picklable, and so I'm not able to send processes to "workers". Could we replace those weakref by something else that would be pickable?

gschwind · 2022-05-13T08:14:10Z

Hello huard,

Why you do not use json serialization as it's done currently to store request in data base ?

Best regards.

huard · 2022-05-13T13:30:21Z

I tried to add __setstate__ and getstatemethod using thejsonproperty andfrom_json` method, but it broke pickling. Replacing the weakrefs by normal references solved this issue, but I wanted to know what were the downsides. I might be missing something obvious. I'll open a separate issue about PyWPS object serialization.

gschwind · 2022-05-13T14:45:07Z

I guess there is downside, I don't think I use weakref if there is no need of it. I have to check. The two main reason to use it. One is for breaking circular dependency with object references such as A->B->A that cause memory leak and require garbage collector to fix it. Second is to not take the ownership of the variableand being able to check if the owner destroyed it (released it).

I have to check. In other hand I would not use pickle to transfer python object from one process to another, but I do not know your use case.

Best regards.

huard · 2022-05-13T15:51:31Z

Ok thanks, see #658.

gschwind · 2022-05-14T09:47:36Z

Hello,

After a quick check, it is for breaking circular references. for instance within the code IOHandler._iohandler is a refecence to FileHandler or DataHandler and those object has FileHandler._ref and DataHandler._ref that is linked to there owner IOHandler. If I do not use weakref here I have a reference loop where IOHandler own FileHandler or DataHandler, and those FileHandler or DataHandler own their parent IOHandler.

If you want as dirty fix, you can use prepare_serialization that remove the weak ref with None and fix_deserialize that rebuild the weak_ref. like:

def prepare_serialization(self):
   self._iohandler._ref = None

def fix_deserialize(self):
    self._iohandler._ref = weakref.ref(self)

Best regards

huard · 2022-05-16T18:23:01Z

Many thanks for the explanation and suggestion.

gschwind force-pushed the pywps-4.4-007 branch 2 times, most recently from cf3b806 to 9f0ee7f Compare April 25, 2021 12:51

cehbrecht requested a review from huard April 26, 2021 13:45

huard approved these changes Apr 26, 2021

View reviewed changes

huard reviewed Apr 26, 2021

View reviewed changes

gschwind force-pushed the pywps-4.4-007 branch from 5b370fe to 3836463 Compare April 27, 2021 19:29

cehbrecht mentioned this pull request Apr 28, 2021

using pytest ... xfail online opendap tests #605

Merged

1 task

gschwind and others added 5 commits April 28, 2021 20:10

Use weakref in the implementation of IOHandler

59246f7

added size property. Adjusted tests to avoid use of private attributes.

09cde34

Remove the obsolet extend_instance function

50919f6

Remove the obsolete IOHandler.prop.setter

babc8fc

gschwind force-pushed the pywps-4.4-007 branch from e185ca2 to babc8fc Compare April 28, 2021 18:41

cehbrecht merged commit 343d825 into geopython:pywps-4.4 May 6, 2021

fmigneault added a commit to crim-ca/weaver that referenced this pull request May 11, 2021

patch pywps-4.4.3 with workaround to avoid file prefer during validat…

2da0a71

…ion (relates to geopython/pywps#526, caused by geopython/pywps@343d825, PR geopython/pywps#602)

fmigneault added a commit to crim-ca/weaver that referenced this pull request May 11, 2021

patch pywps-4.4.3 with workaround to avoid file prefetch during valid…

7efa660

…ation (relates to geopython/pywps#526, caused by geopython/pywps@343d825, PR geopython/pywps#602)

fmigneault mentioned this pull request May 11, 2021

patch pywps-4.4.3 + workaround to avoid file prefetch during validation crim-ca/weaver#251

Merged

gschwind deleted the pywps-4.4-007 branch July 20, 2022 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the implementation of IOHandler #602

Simplify the implementation of IOHandler #602

gschwind commented Apr 25, 2021

huard left a comment

huard Apr 26, 2021

gschwind Apr 27, 2021

huard commented Apr 26, 2021 •

edited

Loading

gschwind commented Apr 27, 2021

coveralls commented Apr 27, 2021 •

edited

Loading

gschwind commented Apr 27, 2021

cehbrecht commented May 6, 2021

huard commented May 12, 2022

gschwind commented May 13, 2022

huard commented May 13, 2022

gschwind commented May 13, 2022

huard commented May 13, 2022

gschwind commented May 14, 2022 •

edited

Loading

huard commented May 16, 2022

Simplify the implementation of IOHandler #602

Simplify the implementation of IOHandler #602

Conversation

gschwind commented Apr 25, 2021

Overview

Related Issue / Discussion

Additional Information

Contribution Agreement

huard left a comment

Choose a reason for hiding this comment

huard Apr 26, 2021

Choose a reason for hiding this comment

gschwind Apr 27, 2021

Choose a reason for hiding this comment

huard commented Apr 26, 2021 • edited Loading

gschwind commented Apr 27, 2021

coveralls commented Apr 27, 2021 • edited Loading

gschwind commented Apr 27, 2021

cehbrecht commented May 6, 2021

huard commented May 12, 2022

gschwind commented May 13, 2022

huard commented May 13, 2022

gschwind commented May 13, 2022

huard commented May 13, 2022

gschwind commented May 14, 2022 • edited Loading

huard commented May 16, 2022

huard commented Apr 26, 2021 •

edited

Loading

coveralls commented Apr 27, 2021 •

edited

Loading

gschwind commented May 14, 2022 •

edited

Loading