Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADD] progress: Decorator for progressbar #233

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[ADD] progress: Decorator for progressbar #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
961c581
[ADD] progress: Decorator for progressbar
pedrobaeza Mar 28, 2021
be10c08
[FIX] add new progressbar2 dependency
yajo May 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions openupgradelib/openupgrade.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import inspect
import uuid
import logging as _logging_module
import functools
from datetime import datetime
try:
from StringIO import StringIO
Expand Down Expand Up @@ -129,6 +130,7 @@ def do_raise(error):
__all__ = [
'migrate',
'logging',
'progress',
'load_data',
'add_fields',
'copy_columns',
Expand Down Expand Up @@ -1876,6 +1878,52 @@ def wrapped_function(cr, version):
return wrap


def progress(index=1, prefix=None):
"""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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@legalsylvain legalsylvain Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

# some custom code
...

@openupgrade.migrate()
def migrate(env, version):
records = ... # get an iterable
migrate_some_stuff(env, records)
"""
def wrap(func):
@functools.wraps(func)
def wrapped_function(*args, **kwargs):
import progressbar
elems = args[index]
prefix2 = prefix or str(func.__name__) + ": "
with progressbar.ProgressBar(
prefix=prefix2, max_value=len(elems)
) as bar:
for elem in elems:
args_copy = list(args)
pedrobaeza marked this conversation as resolved.
Show resolved Hide resolved
args_copy[index] = elem
func(*args_copy, **kwargs)
bar.update(bar.value + 1)
return wrapped_function
return wrap


def move_field_m2o(
cr, pool,
registry_old_model, field_old_model, m2o_field_old_model,
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ coveralls
flake8
pep8-naming
lxml<=4.3.4
progressbar2
psycopg2==2.7.3.1
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
packages=['openupgradelib'],
package_dir={'openupgradelib': 'openupgradelib'},
include_package_data=True,
install_requires=["lxml", "cssselect"],
install_requires=["lxml", "cssselect", "progressbar2"],
license=openupgradelib.__license__,
zip_safe=False,
keywords='openupgradelib',
Expand Down