kopf icon indicating copy to clipboard operation
kopf copied to clipboard

[Bug, Question] - Use decorator on class methods

Open Leletir opened this issue 4 years ago • 10 comments

Keywords

question, decorator, handler, class

Problem

Hello,

First of all, thank you for your work !! This package is so valuable !

I'm trying to create handlers within a class:

class MyHandler:

    def __init__(self):
        ...


    @kopf.on.create("UserService")
    def create_fn(self, spec, name: str, namespace: str, logger, **kwargs) -> None:
        ...

But each time the create_fn method is called I obtain:

[2021-10-16 17:17:56,590] kopf._core.reactor.r [DEBUG   ] Starting Kopf 1.35.1.
[2021-10-16 17:17:56,590] kopf._core.engines.a [INFO    ] Initial authentication has been initiated.
[2021-10-16 17:17:56,590] kopf.activities.auth [DEBUG   ] Activity 'login_via_client' is invoked.
[2021-10-16 17:17:56,598] kopf.activities.auth [DEBUG   ] Client is configured via kubeconfig file.
[2021-10-16 17:17:56,599] kopf.activities.auth [INFO    ] Activity 'login_via_client' succeeded.
[2021-10-16 17:17:56,599] kopf._core.engines.a [INFO    ] Initial authentication has finished.
[2021-10-16 17:17:56,718] kopf._cogs.clients.w [DEBUG   ] Starting the watch-stream for customresourcedefinitions.v1.apiextensions.k8s.io cluster-wide.
[2021-10-16 17:17:56,719] kopf._cogs.clients.w [DEBUG   ] Starting the watch-stream for userservices.v1.sparrow.datalab.pf cluster-wide.
[2021-10-16 17:17:56,828] kopf.objects         [DEBUG   ] Sleeping for 5.169398 seconds for the delayed handlers.
[2021-10-16 17:18:01,999] kopf.objects         [DEBUG   ] Patching with: {'metadata': {'annotations': {'kopf.zalando.org/touch-dummy': '2021-10-16T15:18:01.999238'}}, 'status': {'kopf': {'dummy': '2021-10-16T15:18:01.999238'}}}
[2021-10-16 17:18:02,120] kopf.objects         [DEBUG   ] Creation is in progress: {{'UserServiceHandler.create_fn': {'delayed': '2021-10-16T15:18:01.997562', 'failure': False, 'message': "create_fn() missing 1 required positional argument: 'self'", 'purpose': 'create', 'retries': 17, 'started': '2021-10-16T15:00:53.279299', 'success': False}}}}}
[2021-10-16 17:18:02,120] kopf.objects         [DEBUG   ] Handler 'UserServiceHandler.create_fn' is invoked.
[2021-10-16 17:18:02,123] kopf.objects         [ERROR   ] Handler 'UserServiceHandler.create_fn' failed with an exception. Will retry.
Traceback (most recent call last):
  File "/.../venv/lib/python3.9/site-packages/kopf/_core/actions/execution.py", line 283, in execute_handler_once
    result = await invoke_handler(
  File "/.../venv/lib/python3.9/site-packages/kopf/_core/actions/execution.py", line 378, in invoke_handler
    result = await invocation.invoke(
  File "/.../venv/lib/python3.9/site-packages/kopf/_core/actions/invocation.py", line 140, in invoke
    await asyncio.shield(future)  # slightly expensive: creates tasks
  File "/home/leletir/.pyenv/versions/3.9.2/lib/python3.9/concurrent/futures/thread.py", line 52, in run
    result = self.fn(*self.args, **self.kwargs)
TypeError: create_fn() missing 1 required positional argument: 'self'

I'm using python 3.9.2, kubernetes 1.19.15 and kopf 1.35.1.

If it's a bug, I will be more than happy to give you some help to resolve it.

Have a good day

Leletir avatar Oct 16 '21 15:10 Leletir

I'm hitting the same issue with kopf 1.35.2 with python 3.10.

I've not investigated deeper yet as I'm only doing my first experiments with kopf (which looks great, btw!).

sbidoul avatar Nov 03 '21 15:11 sbidoul

Hello.

In the suggested case of a regular or class method, what would self be? Who and at which point will instantiate the object? How does it align with the operator's lifecycle? What is supposed to happen if there will be 2 (or 10) objects created but only one operator? Should that become 2-10 separate handlers to be invoked (with different self values)?

I would appreciate an example of how this is supposed to be used.

If you only need a namespace, and raw modules are not suitable for you for whatever reason, you can make it a @staticmethod — then it will not require self (I have just checked — it works).

import kopf

class Cls:
    @staticmethod
    @kopf.on.create('kopfexamples')
    def create_fn(spec, **kwargs):
        print(f"And here we are! Creating: {spec}")
        return {'message': 'hello world'}  # will be the new status

If needed, I can make it unfold the "staticmethod" objects back into the "function" object — this will allow the reverse order of decorators: @on-decorators first, @staticmethod last. But it will be the same behaviour without self/cls injected.

nolar avatar Nov 03 '21 19:11 nolar

Looking closer at the principle of operation of kopf, decorating class methods does not make sense indeed. Sorry for the noise.

sbidoul avatar Nov 05 '21 09:11 sbidoul

@nolar With your example above, the final object looks like:

...
status:
  Cls.create_fn:
    message: hello world

How can we adjust the key used to populate the status block, eg, so it's not Cls.create_fn? Is there some kind of arg that can be passed to kopf.on.create? I could not find this easily in the docs.

brianthelion avatar Nov 10 '21 23:11 brianthelion

That is the “id=“ option to all decorators.

For function results, you can instead use the patch kwarg and store any values in any fields. E.g., patch.status[‘result’] = “hello world”.

nolar avatar Nov 10 '21 23:11 nolar

@nolar @sbidoul Returning to the original question now that I understand the decorator idiom a little better, I believe classmethod support can be useful for reducing typing in the staticmethod.

For example, right now I have to do:

class FooBarBazZha:
  SOME_CONST = 1
  @staticmethod
  @kopf.on.create(...)
  def create_fn(...):
    return {"some_var": FooBarBazZha.SOME_CONST}

This could become:

class Foo:
  SOME_CONST = 1
  @classmethod
  @kopf.on.create(...)
  def create_fn(cls, ...):
    return {"some_var": cls.SOME_CONST}

This pattern would provide better support for inheritance and tight encapsulation.

brianthelion avatar Nov 14 '21 16:11 brianthelion

So, you mean using classes as namespaces, right? I am not sure this is the Pythonic way, but I saw it used in some apps & libs.

I would appreciate some clarification regarding this statement:

better support for inheritance and tight encapsulation

What would be the expected behaviour in this sample below? The class Bar also has the inherited create_fn method — in addition to its own another_fn. As a result, there are 2 functions but 3 bound methods are possible:

  • Foo.create_fn — as create_fn(Foo, ...).
  • Bar.create_fn — as create_fn(Bar, ...).
  • Bar.another_fn — as another_fn(Bar, ...).

When a resource is created in Kubernetes, which handlers should be called with which values of cls? The choice between the 1st & the 2nd options affects which value of the constant the handler will see.

class Foo:
  SOME_CONST = 1
  @classmethod
  @kopf.on.create(...)
  def create_fn(cls, ...):
    return {"some_var": cls.SOME_CONST}

class Bar(Foo):
  SOME_CONST = 2
  @classmethod
  @kopf.on.create(...)
  def another_fn(cls, ...):
    return {"some_var": cls.SOME_CONST}

nolar avatar Nov 14 '21 17:11 nolar

@nolar

So, you mean using classes as namespaces, right? I am not sure this is the Pythonic way, but I saw it used in some apps & libs.

I would appreciate some clarification regarding this statement:

better support for inheritance and tight encapsulation

The current decorator pattern with kopf is the same as the decorator pattern used in the early days of MVC web frameworks, especially pylons and its immediate predecessors. (Long "inheritance" story there too.) Framework authors and users quickly realized that this pattern encouraged pollution of the module namespace and created difficulties for (decorated) function re-use.

What would be the expected behaviour in this sample below? The class Bar also has the inherited create_fn method — in addition to its own another_fn. As a result, there are 2 functions but 3 bound methods are possible:

* `Foo.create_fn` — as `create_fn(Foo, ...)`.

* `Bar.create_fn` — as `create_fn(Bar, ...)`.

* `Bar.another_fn` — as `another_fn(Bar, ...)`.

If I recall correctly -- big "if"; might be incorrect -- all the modern web frameworks have settled on a decorator pattern in which decorators on parent classes are ignored if and only if the method they decorate is overloaded. This is achieved by putting an annotation on the decorated method a-la venusian.

brianthelion avatar Nov 15 '21 14:11 brianthelion

But create_fn is not overloaded, it is implicitly inherited.

Can you please point me to a framework or two with examples of how such things are implemented there? I might borrow some concepts from there.

nolar avatar Nov 15 '21 15:11 nolar

But create_fn is not overloaded, it is implicitly inherited.

Ah, yes, I see the difficulty.

Can you please point me to a framework or two with examples of how such things are implemented there? I might borrow some concepts from there.

Yes, will try to find some clear examples for you. Stay tuned.

brianthelion avatar Nov 15 '21 16:11 brianthelion