Draft: Add a decorator method to record function calls with parameters

[zain-sohail]

Related to #87

[rettigl]

This looks quite neat. I assume this only works for class functions of the core class right now, no? Can we also make this work for methods form other classes? In particular, it would be great if we also could track the final settings selected in the interactive tools.

It also currently fails the tests, but this I suppose is quite easy to fix.

[rettigl]

I've fixed the issues, progressed the workflow recorder through to the subclasses, added storing of the function class, and added tracking to the most relevant workflow items.
Might still need some tweeking.

[zain-sohail]

Oh sorry. I wasn't sure why I was getting an error to merge changes currently but now it makes sense. Anyways, you can see that I made changes to the module to track any function calls within a single python session.

You're welcome to adjust the processor class accordingly, or I can also do it.

[steinnymir]

This looks great!
Its similar to what was attempted in hextof-processor it seems, but much better implemented!

Would only be nice to include testing for it.

Very nice addition!

[daviddoji]

Although I'm a big fan of the "dataclasses" module, to be consistent over the whole package, either I would stick to the way you were generating special methods (very easy) or adopt dataclasses everywhere (too much work).

In any case, LGTM.

[zain-sohail]

Hmm could you elaborate on the special methods? I just thought this is a very good usecase for a dataclass. Perhaps, if you give examples of where else it could be used in our code, it could be implemented. I personally like the clarity they bring to the codebase.

[daviddoji]

Special methods are, for example, all double under methods such as the ones used to initialize a class (def __init__(self, ...).

When I'm writing a class I tend to use the dataclass decorator because it automatically takes care of some of those special methods that are boring to write :)

As far as I can see you only use it here and, from my point of view, it breaks consistency with all the other classes already implemented throughout all the library.

[zain-sohail]

My thought was to use the dataclasses for classes that only hold data and the processing elsewhere. I especially see a good usecase with the Metadata as dataclasses can easily have entries like value and unit (using field). Of course, it's possible to do with dicts but it's not possible to add features to them.
Perhaps I had a wrong idea about dataclasses and you can correct me.

[daviddoji]

As I said, I tend to use them because it's a handy decorator. But it only does that (avoiding to write boilerplate code).

My point was that you tend to use dictionaries, lists and such in other classes, so personally, I would continue with such style also in here. I don't see any problems adding new entries to dictionaries or lists.

But, that's my personal view. I tend to be consistent with the code I write ;)

[zain-sohail]

Would only be nice to include testing for it.

Yes, that's a fair point. I will do so once everything's been resolved.

Very nice addition!

Thanks!

[rettigl]

I don't understand how your new version works. I disbled the changes in all other files, but I get:

TypeError: load() missing 1 required positional argument: 'self'

When I am trying to test it.
The addition I did to your previous code worked fine for me, and also included the class of the funktion.
How do you encompass to access the list of called functions? I found it nice to have it as part of the workflow processor, so that one does not need to manage an additional object, and e.g. feed it back in to propagate it to a metadata collection.

The previous version off course does not work for non-class functions, but actuall I did not see a need for this when checking the workflow.

[zain-sohail]

I don't understand how your new version works. I disbled the changes in all other files, but I get:

TypeError: load() missing 1 required positional argument: 'self'

When I am trying to test it. The addition I did to your previous code worked fine for me, and also included the class of the funktion. How do you encompass to access the list of called functions? I found it nice to have it as part of the workflow processor, so that one does not need to manage an additional object, and e.g. feed it back in to propagate it to a metadata collection.

The previous version off course does not work for non-class functions, but actuall I did not see a need for this when checking the workflow.

I'll put here an example:

class Foo:
    @CallTracker
    def bar(self, x, y=1):
        return x + y

foo = Foo()
foo.bar(1, 2)
foo.bar(1, 9)
foo.bar(2, 2)
print(CallTracker.call_tracker)

should return

[MethodCall(name='bar', args=(1, 2), kwargs={}), MethodCall(name='bar', args=(1, 9), kwargs={}), MethodCall(name='bar', args=(2, 2), kwargs={})]

If we also want to include the classname, changing
self.func.__name__ to self.func.__qualname__ should do the trick.

I made the changes I did before you did, after reading your comment

Can we also make this work for methods form other classes?

So now the wrapper is fully independant from any modules or classes and keeps the state. Before, to keep the state, we'd need to store it seperately in each class.
I'd imagine that instead of keeping track in each class seperately, now we can just append it to the MetadataHandler. Would be much cleaner, wouldn't you say?

[zain-sohail]

TypeError: load() missing 1 required positional argument: 'self'

Okay I see where the error occurs. It seems if we leave the arguments to default, it complains. I'll try to find a fix.

[rettigl]

Have a look at 6b57ead

there I passed the list of calls along to each other class, so that they can access this, similar to the MetaHandler. I think that works sufficiently well, if fixing the class method issue is difficult.

[zain-sohail]

I believe the issue is now resolved.

Currently, the default arguments are not stored in the list. It is possible to do that, if that is a useful feature. Perhaps to easily understand what parameter values were used.

[rettigl]

Did you test that? I understand that this will pass the MethodCall instance instead of the one of the calling function.

At least when trying to instantiate the sed processor now, I get:

AttributeError: 'CallTracker' object has no attribute 'loader'

[zain-sohail]

Yes I did test that, with the same example I provided above.

You are not wrong but the update_wrapper argument basically makes the MethodCall instance into a Foo instance, from what I understood. I used it in the functional wrapper approach as well (but it uses a different function there).

I will try to get it to work with loader and other methods we have.

[zain-sohail]

@daviddoji if you have a better understanding of decorators, would be very helpful :)

[daviddoji]

Never played too much with decorators (just silly ones than do timing). Maybe have a look at some examples in here: https://wiki.python.org/moin/PythonDecoratorLibrary

[steinnymir]

I would also think that you should return self.func(*args,**kwargs) as the decorator should then just operate the function as normal.
It seems to me that adding self adds an argument to the function before calling...

[zain-sohail]

Unfortunately that does not pass the self argument to the method, in the case where you don't provide all the arguments (default ones). This one worked for me for a test case but apparently isn't for the real situation. So definitely buggy and I need to understand better the handling.

[rettigl]

I think it is not related to default arguments. This example reproduces the problem:

class Foo:
    @CallTracker
    def bar(self, x=0, y=1):
        return x + y

    @CallTracker
    def bar2(self, x=0, y=1):
        print(self)
        return self.bar(x,y)

foo = Foo()
foo.bar(1, 2)
foo.bar(1, 9)
foo.bar2(2, 2)
foo.bar2()
print(CallTracker.call_tracker)

Output:

<sed.core.workflow_recorder.CallTracker object at 0x7f27118c6400>
...
AttributeError: 'CallTracker' object has no attribute 'bar'

[rettigl]

Apparently the "self" object passed to decorated functions is that of CallTracker, rather than the parent class.

[rettigl]

The previous version I marked in the commit above (or the following one with linting) should do what we want without these problems, at least it looked like it to me. Only downside I see is that it only works for class methods, but it was anyways the idea to provide wrappers for all relevant functions, no?

[zain-sohail]

Sorry for not continuing this request. I was a bit busy with exams, but seeing that Steinn has a different proposal, we can hold it off for the time being? @steinnymir or do you suggest, I finish this feature?
That'd be either the way I wanted to finish implementing or Laurenz already found working.

[steinnymir]

I am not yet sure how the new version might connect to this. My first impression is it would no longer be required, but I cannot yet say with certainty.

[zain-sohail]

Not relevant with the new architecture.

[zain-sohail]

Since, the new architecture will take time still, I thought we could finish this off and add it to sed. Possibly the branch is so old that making a new branch would be better. But opening for discussion.