New decorator

[cimatosa]

hey @paulmueller

I wonder If you can live with these changes. It might require some changes in your use cases, because count, max_count = None will no longer be treated in a special way. Instead max_count = None will lead to the same behavior as in the progress.ProgressBar, simply ignoring the max_value and use a simple progressCounter instead. Instead of using None you have to plug in some default value like c=UnsignedIntValue(0) and m = UnsignedIntValue(1). see examples in class docstring or test/test_decorators.py

What you think?

cima

[paulmueller]

The way I understand it, I don't like it. I want the docstrings of my algorithms to be as simple as possible. This means having default values of "None", if you don't need it for the algorithm itself.

I would also need to import UnsignedIntValue into the module where my algorithm is defined, which I don't like.

Why do you have to check for the default values of count and count_max? Isn't their existence enough to assume that their .value will be set?

btw I thought

valid = func.__code__.co_varnames[:func.__code__.co_argcount]

was a quite cool hack. It should work with Python3 as well.

[cimatosa]

The thing is, if you plan on using any sort of status in your functions you need to have some code fragments incrementing the counter, or even setting the max value. so if count and max_count are None you need to explicitly check that before setting the value, which I think makes the code ugly. So, If one plans to use a statusbar, then why not have these value set any way, even if they are not used by any progressbar. Further you don't need to import decorators for the UnsignedIntValue, since it is nothing but multiproccessing.Value('I', val, lock=True) and will be provided as python standard library, so your code will also run without jobmanager.

Including count and max_count simply provides an interface to report status information. Independent of the backend which will show the progress. I think that's nice and transparent.

your cool hack is still there, it is hidden in getCountKwargs, but I think you even write this?

[cimatosa]

the two main things I changes are

1) the special treatment of None has gone (which we need to discuss)

2) the call is not just looking in the kwargs for c, m but in the args also. the insprect.getcallargs method does that for us. Try your version by calling a function with c,m set as positional arguments, it will not work. That's why I changed that.

Cheers

[paulmueller]

Hi, sorry for the confusion. The valid = line was just dust that needed to be cleaned up. Thanks.

1) This boils down to what the __call__ method of the decorator does, right? Couldn't we just add another line there to check if the default argument is None and if so, give the function a multiproccessing.Value instance? In which scenario would that be a problem?

2) I understand, this is more universal.

Cheers

[cimatosa]

The prob with the None is that it has already another meaning. namely that there is NO max_value, which changes the behavior of the ProgressBar.

At first glance I was thinking to introduce another special name instead of None like Auto, which is defined in decorators. But that also implies that decorators is imported. That I thought simply using a string like 'auto'. But because of the checking needed, like if count != 'auto' then count += 1, I thought it might be best to just use the UnsignedIntValue as default.

but If you really don't like it, I mean really really, then I would go for the string 'auto' solution?

[paulmueller]

I really really don't like it, because it would break compatibility with all of my packages. Of course I could settle for some string, but that would take some effort revising all of my code.

I am still not convinced. Why should an algorithm that does not set the count_max.value have count_max as an argument? Why does the ProgressBar even care about default arguments?

Could you give me an example?

Maybe we can settle for a keyword argument to decorators.ProgressBar, e.g. replace_counters=True to create mp.Value for each function and replace_counters=False to use the default arguments of the function.

[EDIT] I think the keyword-thing is not a good Idea. It messes up the **kwargs for ProgressBar.

[paulmueller]

Here are three solutions:

1)

• decorators.ProgressBar will always create a new mp.Value and set them to 0 for count and 0 for max_count.
• progress.ProgressBar will treat algorithms that do not set max_count to something >0 after the first counts as a simple progressCounter.

2) Keep everything as-is in the new_decorator branch and add another decorator derived from decorators.ProgressBar with a different __call__ function that overwrites initial values for count and max_count with mp.Valuess. This would only require minor changes in my code.

3) Always treat functions that have max_count as an argument with progress bars. This would probably break some of your code.

[cimatosa]

i like solution 2) the most!

So we will have a decorator obeying the same interface from progressBar, which I think is essential. And then to derive another decorator from this which modifies the behavior to your need sound clean and transparent to me!

cool shit!

[paulmueller]

ok, solution 2 it is. I vote for decorators.ProgressBarOverrideCount which is a hint to where it should be applied. :pizza:

[cimatosa]

ok, what you think ...

[paulmueller]

I added an override_count=False keyword to the module decorator method. If there are no objections from your side, I am happy for this to get merged.

[cimatosa]

sure let's leave is like that. I just thought, that one could also pass the class which will be used as a decorator as argument. In that case it would be easy to use the module decorator with "what-ever" decorator class. But we can also do it later, when it is actually needed!

SO FEEL FREE TO MERGE!

[paulmueller]

There was a conflict, but since everything runs through smoothly, I am assuming nothing broke.