[ADD] progress: Decorator for progressbar

[pedrobaeza]

This is a decorator for any sub function called in an OpenUpgrade script (pre, post or end migration script).

Decorate functions that may take time. If a function is decorated, we provide an iterable argument that will be looped on and call the original function, while a progress bar will be displayed.

Function: 28% (152 of 529) I#####              I Elapsed Time: 0:00:03 ETA: 0:01:32

:param index: Index of the argument to be used as iterable. Default to the second argument. It will pass each of the elements of the iterable in the same place.
:param title: Optional title for prefixing the progress bar. If not specified, the function name will be used.

Typical use:

@openupgrade.progress()
def migrate_some_stuff(env, record)
    # some custom code
    ...

@openupgrade.migrate()
def migrate(env, version):
    records = ...  # get an iterable
    migrate_some_stuff(env, records)

Co-Authored-By: Yann Papouin [email protected] @ypapouin

@Tecnativa TT19674

[pedrobaeza]

See a usage example in OCA/OpenUpgrade@3c4fc81

@yajo @joao-p-marques this means an extra library for openupgradelib. I think we should include progressbar2 in Doodba.

[legalsylvain]

nice addition ! thanks to have worked on that topic !

No tested. (and light code review, because decorators are quite hard to understand for me).

Question : what is the impact, if the log is written in a file ? the progression is written ?

[legalsylvain]

record -> records.

I mean, should be work with one or many records. (if one record, it should be in an interable).
If not, it will not be possible to add / remove the decorator easely.

This could avoid the big diff, introduced here OCA/OpenUpgrade@3c4fc81

Do you think it's pertinent ?

[pedrobaeza]

That can't be possible, Sylvain, as this needs to control each step of the loop for the progress bar. Think on this like @api.one decorator, in which self is only one record. You pass the whole recordset, but what gets to your method is only one. Here I took care of putting the proper nomenclature of the arguments for not confusing.

The only way to get what you want is to intercept things into another level, like initially @ypapouin did in his PR #206, but that means that we need an specific decorator for each kind of operation (his one was for a fetchall), and lose the general purpose. We can override the iterator and create a wrapper, but this still is limited, as you can't have several nested loops, so it's not a lot of problem to change this. Now it seems a bit dirty as applied over a method that previously was not "progressified", but when writing new methods, there won't be diff and even the cyclomatic complexity will be lower, reducing one indentation level.

[legalsylvain]

Hi @pedrobaeza.

thanks for your answer.
In a first sight, I was thinking to replace args_copy[index] = elem by args_copy[index] = [elem]. It can do the job.

• It's not perfect and should ask some minor changes for dict but it avoid huge diff. (dict.items() can not be used inside and should be replaced)
• Also, if using progress bar generates side bad effects for any reasons (performance, in the log, or whatever), it could be great to disable / remove it easily)
• if using the progress bar requires to change the code, it will slow down the use of this new decorator for the current migration scripts.
• when writing function, it is counter intuitive to call a function with a list, and in the function, to have a single element to consume.

In the other hand,

• I like the reduction of the complexity you're talking about
• indeed it will not be a problem for new function

I don't know ! I have mixed feelings on that point. Maybe other openupgrader will will have additional opinions.

Any way, not a blocking point. I think that introducing progressbar is a step in the right direction.

Note : not sure to have been convinced by the @api.one api. Most of the functions call in the first line, the self.ensure_one() command, that is raising an error if a recordset with more than one item is provided.

[pedrobaeza]

I sincerely don't think passing a list of one element is the way to go. It's a bit antinatural and only for these cases where we adapt existing methods to reduce the diff, but it's less readable and put in doubt what the decorator does IMO.

[pedrobaeza]

when writing function, it is counter intuitive to call a function with a list, and in the function, to have a single element to consume.

This is done in other places like migrate(cr, version) that gets converted to migrate(env, version).

[pedrobaeza]

About the impact in the log, it depends on the log technique you use. In principle, stdout is written as much times as progression steps displayed, so that's not ideal, but I haven't investigated much yet about how to avoid it. There's some info about this in the library itself in https://github.com/WoLpH/python-progressbar#progressbars-with-logging.

There's also other progress bar library mentioned frequently, called tqdm (https://github.com/tqdm/tqdm), and I don't know if it's better than this one.

[legalsylvain]

In principle, stdout is written as much times as progression steps displayed, so that's not ideal, but I haven't investigated much yet about how to avoid it

if the log file is polluted, we could add a test like if not tools.config['logfile'].

[joao-p-marques]

@yajo @joao-p-marques this means an extra library for openupgradelib. I think we should include progressbar2 in Doodba.

Ok, but I think that, if the dependency is correctly set up in the Python package (i.e. setup.py or requirements.txt, as you do here), we shouldn't need to change anything.
When installing openupgradelib downstream (in doodba-copier-template, for example: https://github.com/Tecnativa/doodba-copier-template/blob/stable/odoo/custom/dependencies/pip.txt#L1) it should be installed by default.
@yajo can you confirm please?

[pedrobaeza]

if the log file is polluted, we could add a test like if not tools.config['logfile'].

@legalsylvain the logfile is not polluted. I meant that if you do -u all and redirect stdout to a file, then progressbar is logged.

[pedrobaeza]

Found the problem in Travis, that was a flake8 one.

[StefanRijnhart]

This is nice, but would you consider making it optional by checking if a variable like OPENUPGRADE_DISABLE_PROGRESS is defined in the environment? I find myself watching logs in stdout so that I can observe any crashes in the same terminal. Of course, I capture the terminal output to file, and, like Sylvain, I would not like to have this log cluttered.

[pedrobaeza]

Which method do you use for outputting the log to a file for looking for a way of having both?

[StefanRijnhart]

I use tee, or script

[pedrobaeza]

OK, I will check if there's any option.

[StefanRijnhart]

I checked progressbar2, and there doesn't seem to be any. But you can choose to check in this code and if it is set, just execute the method instead of applying the progress bar.

[pedrobaeza]

Yeah, but then we won't benefit from the feature (I also do it this way: ... 2>&1 | tee ...).

[StefanRijnhart]

That was my request, to make the feature optional ;-)

[yajo]

I added the dependency where it was missing.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[leemannd]

Nice feature! @pedrobaeza Are you still willing to push forward and merge it?

[StefanRijnhart]

Still hoping for an addition that allows me to disable this when I set OPENUPGRADE_DISABLE_PROGRESS=1 in the environment.

[legalsylvain]

Need rebase and fix conflict and add options.

[OCA-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖