Cylc-flow: Extra self parameter in mutations

[kinow]

Describe the bug

There's an extra self parameter in the mutation functions. But not on the resolvers.

Release version(s) and/or repository branch(es) affected?

master

Steps to reproduce the bug

NA

Expected behavior

No self as the function is in a module, not in an object.

Screenshots

NA

Additional context

https://github.com/cylc/cylc-flow/blob/d6f9bb0f9a57f76d1caca83f31b8033c6fad03a8/cylc/flow/network/schema.py#L673

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

[kinow]

Maybe it works even with the self... in this case, a) could we move it under a Mutation object or, b) could we just add a comment in these functions to tell other devs the reason for that, and alert them to not remove the self?

[hjoliver]

It looks like a mistake to me. I guess it would only work if calling code used only named args, with self not having its normal special semantics, but actually we are not using these mutator functions anywhere yet?

[kinow]

I think these mutators are used... If you look at this Class: https://github.com/cylc/cylc-flow/blob/146347af9ac115632b3c3efd8848e212599489d6/cylc/flow/network/schema.py#L726

It defines resolver as that mutator function.

It is a bit unconventional I guess. The function is defined elsewhere. @dwsutherland confirmed it works yesterday, but perhaps a base class being extended instead could simplify the code.

To confirm it works (another Today-I-Learned): https://github.com/kinow/notebooks/blob/master/oo/Python%20OO%20dynamic%20defining%20member%20function.ipynb

[hjoliver]

Oh yeah, you're right. Hmmm. :thinking:

[kinow]

Almost forgot, @dwsutherland explained yesterday to me that the reason for that function being defined as a function in the module, was exactly to allow it to be used in several classes.

So both this approach (composition - kinda?) or a base class (hierarchy) would give the same result. I think the inheritance approach would simplify and avoid accidental bugs (forgetting the self in a new function, or accidentally removing the self).

[hjoliver]

Yes it's kind of ugly and non-intuitive, in that a standalone function is defined, but it can only be used inside a class that it doesn't belong to.

[hjoliver]

(Well it can be used as a function, as opposed to a class method, but only if self is treated as a normal variable ... but that would be perverse!)

[hjoliver]

Hang on, self is not actually used inside the functions, so they could just be normal functions, no?

[kinow]

Hang on, self is not actually used inside the functions, so they could just be normal functions, no?

I'm confused.. not sure if you mean to remove self from the function, or to move the function to under a class?

[hjoliver]

Just remove self from the function args ... it's not referenced anywhere in the function.

[hjoliver]

(or have I wildly misunderstood something here!)

[kinow]

Oh, I think I got it. If we remove the self from that function, then once it is "attached" to the class, it becomes an invalid function (unless we make that static). I've updated the notebook to test that (see AnotherDuck): https://github.com/kinow/notebooks/blob/master/oo/Python%20OO%20dynamic%20defining%20member%20function.ipynb

Results in TypeError: quack_1() takes exactly 1 argument (2 given)

Is that what you meant?

[hjoliver]

But why does it need to be "attached to the class" - can't it just be used within the class as an external function?

[hjoliver]

(Or is the code somewhere expecting it to be a class method, and we can't change that?)

[kinow]

But why does it need to be "attached to the class" - can't it just be used within the class as an external function?
(Or is the code somewhere expecting it to be a class method, and we can't change that?)

I think so (@dwsutherland correct me if I'm wrong). Though their docs on mutations seem to use mutate and not resolve. Also, in the docs the method is static (though in one example it doesn't have the static decorator) :confused:

[hjoliver]

OK, we'll wait on @dwsutherland to pitch in on this! (when he's available).

[dwsutherland]

I changed self to root.. Because the resolver functions are used in the same way:
https://github.com/dwsutherland/cylc-flow/blob/bb9995e7aa157f81312c5731838280687399f3ac/cylc/flow/network/schema.py#L243
And root could be used like the resolvers to introspect the parent:

async def get_nodes_by_id(root, info, **args):
    """Resolver for returning job, task, family node"""
    field_name = to_snake_case(info.field_name)
    field_ids = getattr(root, field_name, None)
    if hasattr(args, 'id'):
.
.
.

It's a documented approach:
https://docs.graphene-python.org/en/latest/types/objecttypes/#resolvers-outside-the-class
They are implicit static methods:
https://docs.graphene-python.org/en/latest/types/objecttypes/#implicit-staticmethod
So I think I shouldn't have call it self in the first place!

The Graphene documentation has been improved a lot in the last year, I need to read it again!
(I spent a lot of time reading the source code back then..)

[dwsutherland]

Also root has a special meaning, this link gets to the heart of it:
https://docs.graphene-python.org/en/latest/types/objecttypes/#naming-convention

[hjoliver]

A-ha, yes:

Sometimes this argument will be named self in Graphene code, but this can be misleading due to Implicit staticmethod while executing queries in Graphene.

[kinow]

So I think it's all clear now, once the PR gets merged and self becomes root, this one can be closed. Thanks for clarifying @dwsutherland !

[hjoliver]

Definitely worth an explanatory comment in the source code though @dwsutherland