Francesco Romani has uploaded a new change for review.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
lib: api.py: preserve signature in decorator
The api.method decorator needs to preserve the function or method signature, to let the schema validation keep working. Moreover, preserving the signature of decorated callables is a good idea anyway.
In python 3.4+ this is done by default, but on python 2.7 we need to take steps to implement this. We leverage the existing 'decorator' module which solves this problem. This module is available in EL/Centos7.
For further reference: http://blog.dscpl.com.au/2014/01/how-you-implemented-your-python.html http://www.phyast.pitt.edu/~micheles/python/documentation.html
Change-Id: I06205cf180229ea19ffc38a7b88346afc18f13e4 Signed-off-by: Francesco Romani fromani@redhat.com --- M lib/vdsm/common/api.py M vdsm.spec.in 2 files changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/59/63759/1
diff --git a/lib/vdsm/common/api.py b/lib/vdsm/common/api.py index 354b665..76a75c3 100644 --- a/lib/vdsm/common/api.py +++ b/lib/vdsm/common/api.py @@ -22,6 +22,8 @@ from functools import wraps import logging
+from decorator import decorator + from vdsm import exception from vdsm import response
@@ -29,6 +31,7 @@ _log = logging.getLogger("virt.api")
+@decorator def method(func): """ Decorate an instance method, and return a response according to the diff --git a/vdsm.spec.in b/vdsm.spec.in index 26c36d3..7af1036 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -136,6 +136,7 @@ Requires: ntp Requires: iproute >= 3.10.0 Requires: PyYAML +Requires: python-decorator Requires: python-netaddr Requires: python-inotify Requires: python-argparse
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 2:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 3:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Jenkins CI has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 3: Continuous-Integration-1
Propagate review hook: Continuous Integration value inherited from patch 2
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 4:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 5:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 5: Verified+1
verified with the tests
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 6:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 6: Verified+1
I used decorator.decorate, but in EL7 we have the old 3.4.0 package, which doesn't provide that. So in PS6 I switched back to decorator.decorator. Tests still pass on my FC24, let's see how it goes on EL7.
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/63759/6/vdsm.spec.in File vdsm.spec.in:
Line 135: Requires: xz Line 136: Requires: ntp Line 137: Requires: iproute >= 3.10.0 Line 138: Requires: PyYAML Line 139: Requires: python-decorator this must also be added to BuildRequires Line 140: Requires: python-netaddr Line 141: Requires: python-inotify Line 142: Requires: python-argparse Line 143: Requires: python-cpopen >= 1.4
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7: Verified+1
in PS7 I fixed the dependencies (hopefully), and I dealt with the ugly naming differences. In EL7 the package is python-decorator; in FC24, we have both python2-decorator and python3-decorator.
Nir Soffer has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
(3 comments)
https://gerrit.ovirt.org/#/c/63759/7/lib/vdsm/common/api.py File lib/vdsm/common/api.py:
Line 29: Line 30: _log = logging.getLogger("virt.api") Line 31: Line 32: Line 33: @decorator Nice!
Sad that we need such hacks for such basic code, but we probably need this on all decorators.
Did you inspect the decorator module code and tested its performance compared with old code?
For example, I would like to see timeit output before and after this patch, calling decorated method. Line 34: def method(func, *args, **kwargs): Line 35: """ Line 36: Decorate an instance method, and return a response according to the Line 37: outcome of the call.
https://gerrit.ovirt.org/#/c/63759/7/vdsm.spec.in File vdsm.spec.in:
Line 109: %endif # support python3 Line 110: Line 111: %if 0%{?rhel} Line 112: BuildRequires: python-decorator Line 113: %else # fedora Please have explicit branch for each platform, no more "else # fedora", this starts small, and quickly becomes impossible to maintain. Line 114: BuildRequires: python2-decorator Line 115: %endif # rhel Line 116: Line 117: # Autotools BuildRequires
Line 144: Requires: iproute >= 3.10.0 Line 145: Requires: PyYAML Line 146: %if 0%{?rhel} Line 147: Requires: python-decorator Line 148: %else # fedora Same - best to have one block for platform specific requirements , not block per requirement, maybe at the end of the this list? Line 149: Requires: python2-decorator Line 150: %endif # rhel Line 151: Requires: python-netaddr Line 152: Requires: python-inotify
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/63759/7/lib/vdsm/common/api.py File lib/vdsm/common/api.py:
Line 29: Line 30: _log = logging.getLogger("virt.api") Line 31: Line 32: Line 33: @decorator
Nice!
I didn't though about performance, but will check the numbers, now I'm curious as well. Line 34: def method(func, *args, **kwargs): Line 35: """ Line 36: Decorate an instance method, and return a response according to the Line 37: outcome of the call.
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
(2 comments)
https://gerrit.ovirt.org/#/c/63759/7/vdsm.spec.in File vdsm.spec.in:
Line 109: %endif # support python3 Line 110: Line 111: %if 0%{?rhel} Line 112: BuildRequires: python-decorator Line 113: %else # fedora
Please have explicit branch for each platform, no more "else # fedora", thi
Done Line 114: BuildRequires: python2-decorator Line 115: %endif # rhel Line 116: Line 117: # Autotools BuildRequires
Line 144: Requires: iproute >= 3.10.0 Line 145: Requires: PyYAML Line 146: %if 0%{?rhel} Line 147: Requires: python-decorator Line 148: %else # fedora
Same - best to have one block for platform specific requirements , not bloc
Done Line 149: Requires: python2-decorator Line 150: %endif # rhel Line 151: Requires: python-netaddr Line 152: Requires: python-inotify
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 8:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/63759/7/lib/vdsm/common/api.py File lib/vdsm/common/api.py:
Line 29: Line 30: _log = logging.getLogger("virt.api") Line 31: Line 32: Line 33: @decorator
I didn't though about performance, but will check the numbers, now I'm curi
The microbenchmark (https://gerrit.ovirt.org/63833) doesn't look too good:
decorator method speed penalty: with just functools.wraps: 0.00645804405212 with the decorator module: 0.0969560146332 Line 34: def method(func, *args, **kwargs): Line 35: """ Line 36: Decorate an instance method, and return a response according to the Line 37: outcome of the call.
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 9:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/63759/7/lib/vdsm/common/api.py File lib/vdsm/common/api.py:
Line 29: Line 30: _log = logging.getLogger("virt.api") Line 31: Line 32: Line 33: @decorator
The microbenchmark (https://gerrit.ovirt.org/63833) doesn't look too good:
The slowdown is 60 usec per call - not interesting, and the code is much nicer with this. Line 34: def method(func, *args, **kwargs): Line 35: """ Line 36: Decorate an instance method, and return a response according to the Line 37: outcome of the call.
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/63759/7/lib/vdsm/common/api.py File lib/vdsm/common/api.py:
Line 29: Line 30: _log = logging.getLogger("virt.api") Line 31: Line 32: Line 33: @decorator
The slowdown is 60 usec per call - not interesting, and the code is much ni
ok, furthermore we agreed that this wrapping will go ahead in the mid/long term. Line 34: def method(func, *args, **kwargs): Line 35: """ Line 36: Decorate an instance method, and return a response according to the Line 37: outcome of the call.
Nir Soffer has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 7:
(1 comment)
https://gerrit.ovirt.org/#/c/63759/7/lib/vdsm/common/api.py File lib/vdsm/common/api.py:
Line 29: Line 30: _log = logging.getLogger("virt.api") Line 31: Line 32: Line 33: @decorator
ok, furthermore we agreed that this wrapping will go ahead in the mid/long
I'm thinking about using this in other places in vdsm, the standard decorator boilerplate is not fun to write or maintain. Line 34: def method(func, *args, **kwargs): Line 35: """ Line 36: Decorate an instance method, and return a response according to the Line 37: outcome of the call.
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 10:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 11:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 11: Code-Review+1
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 12:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Piotr Kliczewski has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 12: Code-Review+1
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 13:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Francesco Romani has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 13: Verified+1
schemaValidation passes with this patch.
Dan Kenigsberg has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 13: Code-Review+2
never mind the minor slowdown of each API call, let's take this in.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
lib: api.py: preserve signature in decorator
The api.method decorator needs to preserve the function or method signature, to let the schema validation keep working. Moreover, preserving the signature of decorated callables is a good idea anyway.
In python 3.4+ this is done by default, but on python 2.7 we need to take steps to implement this. We leverage the existing 'decorator' module which solves this problem. This module is available in EL/Centos7.
For further reference: http://pythonhosted.org/decorator/documentation.html
Change-Id: I06205cf180229ea19ffc38a7b88346afc18f13e4 Signed-off-by: Francesco Romani fromani@redhat.com Reviewed-on: https://gerrit.ovirt.org/63759 Reviewed-by: Nir Soffer nsoffer@redhat.com Reviewed-by: Piotr Kliczewski piotr.kliczewski@gmail.com Continuous-Integration: Jenkins CI Reviewed-by: Dan Kenigsberg danken@redhat.com --- M automation/build-artifacts.packages.el7 M automation/build-artifacts.packages.fc24 M automation/check-merged.packages.el7 M automation/check-merged.packages.fc24 M automation/check-patch.packages.el7 M automation/check-patch.packages.fc24 M lib/vdsm/common/api.py M tests/api_test.py M vdsm.spec.in 9 files changed, 55 insertions(+), 20 deletions(-)
Approvals: Piotr Kliczewski: Looks good to me, but someone else must approve Nir Soffer: Looks good to me, but someone else must approve Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Francesco Romani: Verified
gerrit-hooks has posted comments on this change.
Change subject: lib: api.py: preserve signature in decorator ......................................................................
Patch Set 14:
* Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
vdsm-patches@lists.fedorahosted.org