[WIP][ENH] Easy way to prevent unneeded field recomputation.

[NL66278]

Description of the issue/feature this PR addresses: Recomputation of fields can take a long time, which is problematic when those fields will be filled with the correct values anyway.

Current behavior before PR: Only way to prevent long running unneeded computations was to create fields through SQL, which has the danger of not all attributes taken into account (like indexes) etc. Also takes a lot of code.

Desired behavior after PR is merged: It will be easy to supress feld recompute.

Example use:

def blacklist_field_recomputation(env):                                        
    from openerp.addons.account.models.account_invoice import AccountInvoice
    AccountInvoice._openupgrade_recompute_fields_blacklist = [
        'payment_move_line_ids',
        'price_subtotal_signed',
    ]


--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

[pedrobaeza]

Another way to prevent this recomputation is just created the SQL column on pre-migration script. This is done all across OpenUpgrade.

[NL66278]

@pedrobaeza I know and addressed this in the description of the PR. The precreation has the disadvantage that you have to do a lot of work to exactly match how the field would be created by the Odoo ORM. (including creating indexes where needed, etc.) With this PR we can just have the Odoo ORM do its work. It is in my opinion more reliable and a lot less work.

[pedrobaeza]

OK, I didn't know about index creation (and I'm not sure about that part either). The problem with this is that you will need to forward-port and maintain this method for each OpenUpgrade version, and including in openupgradelib is not an option as you can imagine.

[pedrobaeza]

Why not _auto=False for avoiding table creation?

[NL66278]

I changed to _auto and a normal Model. Thanks for the suggestion!

[StefanRijnhart]

How about adding the backlist as a property to models.BaseModel in openerp/models.py and check for that?

[StefanRijnhart]

In your pre script, you might be able to do:

from opener.addons.sale.models.sale_order import sale_order
sale_order._recompute_fields_blackist = ['some_field']

You should probably reset the property in the post script.

[NL66278]

@StefanRijnhart Just wondered how to set and use the property. When actually looking at the blacklist, the fields are recomputed for all models. Obviously I can determine the model, look it up in the registry, then look at the property, but would this not be more work/code then what I have now?

[StefanRijnhart]

Retrieve the model's blacklist from the recs variable.

Proof of concept:

diff --git a/openerp/addons/base/__openerp__.py b/openerp/addons/base/__openerp__.py
index 5a9936c71eb..8017d64d055 100644
--- a/openerp/addons/base/__openerp__.py
+++ b/openerp/addons/base/__openerp__.py
@@ -4,7 +4,7 @@
 
 {
     'name': 'Base',
-    'version': '1.3',
+    'version': '1.4',
     'category': 'Hidden',
     'description': """
 The kernel of OpenERP, needed for all installation.
diff --git a/openerp/addons/base/migrations/1.4/pre-migrate.py b/openerp/addons/base/migrations/1.4/pre-migrate.py
new file mode 100644
index 00000000000..b09c763ae2a
--- /dev/null
+++ b/openerp/addons/base/migrations/1.4/pre-migrate.py
@@ -0,0 +1,5 @@
+from openerp.addons.base.ir.ir_ui_menu import ir_ui_menu
+
+
+def migrate(cr, version):
+    ir_ui_menu._blacklist.append('web_icon_data')
diff --git a/openerp/models.py b/openerp/models.py
index 2af2ec78d09..e3c74eb6b98 100644
--- a/openerp/models.py
+++ b/openerp/models.py
@@ -301,6 +301,7 @@ class BaseModel(object):
     To create a class that should not be instantiated, the _register class
     attribute may be set to False.
     """
+    _blacklist = []
     __metaclass__ = MetaModel
     _auto = True # create database backend
     _register = False # Set to false if the model shouldn't be automatically discovered.

At the point where you propose to intervene in method recompute():

> /home//buildout/9.0/parts/odoo/openerp/models.py(5876)recompute()
-> vals = rec._convert_to_write({n: rec[n] for n in ns})
(Pdb) rec
ir.ui.menu(1,)
(Pdb) rec._blacklist
['web_icon_data']

[StefanRijnhart]

BTW for the sake of clarity you could call it _openupgrade_recompute_fields_blacklist or something.

[NL66278]

@StefanRijnhart I rewrote the blacklist proposal according to your suggestions.

Example of use in PR (that includes this PR): #663

[StefanRijnhart]

Thanks!

[StefanRijnhart]

Looks like this resets the blacklist after every write. Is that what you want?

[NL66278]

@StefanRijnhart Actually not after every write, but after handling all the changes required by migrating one module. This takes care of your remark that the property will need to be reset in the post-migration, but without the need to do this manually. Otherwise the blacklist might still be active in other modules being migrated (or if the user actually logs into the migrated server to check the migration results).

[StefanRijnhart]

Am I missing something? On an Odoo 9.0 database with only 'base' ínstalled without demo data I have this patch:

--- a/openerp/models.py
+++ b/openerp/models.py
@@ -5863,6 +5863,7 @@ class BaseModel(object):
         """ Recompute stored function fields. The fields and records to
             recompute have been determined by method :meth:`modified`.
         """
+        print "Recompute called"
         while self.env.has_todo():
             field, recs = self.env.get_todo()
             # determine the fields to recompute

if I do --update all, this is printed 1854 times.

[NL66278]

@StefanRijnhart Good catch. Other calls were due to database updates done during migration. Although code worked in practice, I moved the reset to a more appropriate place (I hope).

[StefanRijnhart]

Thanks, looks like a useful addition! Upon merge this should be squashed for easy cherrypicking at least into 10.0.

[NL66278]

I found an issue that the blacklist is not always removed, because not all updated models are in the object list.

[StefanRijnhart]

Setting to work in progress as per your latest comment.

[mvaled]

Hi @NL66278,

Regarding what you have said about "an issue that the blacklist is not always removed, because not all updated models are in the object list." could you give me more details about the issue so that I can see if we can tackle it.

[NL66278]

Hi @mvaled When some models have blacklisted fields, but the migrated database does not require recomputation of a field in that model, the fields remain blacklisted, because they are not in the list used at this point: https://github.com/OCA/OpenUpgrade/pull/735/files#diff-a574457167ec463fe510488c27d95214L317 When in subsequent modules those fields do need recomputation, they are still in the list, and therefore not recomputed.

[mvaled]

So perhaps the approach would be to add/remove the blacklist at the migration boundaries. I'll be looking into it after a couple of hours I'm in the middle of a tough merge request from a colleague.

[mvaled]

Hi @NL66278,

I'm a bit confused about your conclusion.

I see that init_module_models is being called while loading the modules. The models (which becomes obj_list inside the init_module_models()) passed are those returned from the registry.load() a few lines above.

Reading the code of Registry.load() I see that every single model defined in such addon will be in the obj_list. So, unless we're blacklisting a model not defined in the current addon, it will be cleared. If we need to blacklist models from other modules, this can be easily avoided by declaring a 'empty' stub for the model.

Am I right, or am I missing something?

[mvaled]

Hi @NL66278

Could you comment on my last remark in this PR?

[NL66278]

@mvaled You are missing something. I initially made the same assumptions as you. However when no recomputation takes place on a model, might be because there are no records needing recomputation, this model will not be in the obj_list.

[mvaled]

@NL66278 I think that happened before b9c5e6f. Before it, the removal of blacklisted fields was implemented in the recompute method, so if no recomputation was done, backlists were not purged. But in b9c5e6f you moved that code into init_module_models which is called while loading the graph of modules. There, I think, blacklists will always be purged after upgrading module, but only for models that were defined (or modified) in the same module.

I'm doing a little tracing with the following modification:

diff --git a/openerp/modules/module.py b/openerp/modules/module.py
index a8676a5754b..278ff952488 100644
--- a/openerp/modules/module.py
+++ b/openerp/modules/module.py
@@ -308,6 +308,7 @@ def init_module_models(cr, module_name, obj_list):
 
     """
     _logger.info('module %s: creating or updating database tables', module_name)
+    _logger.debug('module %s models: %r', module_name, obj_list)
     todo = []
     for obj in obj_list:
         result = obj._auto_init(cr, {'module': module_name})
@@ -322,6 +323,7 @@ def init_module_models(cr, module_name, obj_list):
     todo.sort(key=lambda x: x[0])
     for t in todo:
         t[1](cr, *t[2])
+    _logger.debug('module %s: finalizing and (possibly) reseting blacklists for %r', module_name, obj_list)
     if obj_list:
         # OpenUpgrade: Don't trigger workflows on recomputation
         set_workflow_org = openerp.models.BaseModel.step_workflow

With that in place I see that modules where no recomputation is needed do log the finalizing message:

2017-06-22 18:05:31,010 10912 DEBUG o9-mercurio openerp.modules.module: module database_cleanup models: [cleanup.purge.line, cleanup.purge.wizard, ir.model.data, cleanup.purge.line.module, cleanup.purge.wizard.module, ir.model, cleanup.purge.line.model, cleanup.purge.wizard.model, cleanup.purge.line.column, cleanup.purge.wizard.column, cleanup.purge.line.table, cleanup.purge.wizard.table, cleanup.purge.line.data, cleanup.purge.wizard.data, cleanup.purge.line.menu, cleanup.purge.wizard.menu, ir.model.fields, cleanup.create_indexes.line, cleanup.create_indexes.wizard, cleanup.purge.line.property, cleanup.purge.wizard.property]
2017-06-22 18:05:31,061 10912 DEBUG o9-mercurio openerp.models.schema: Table 'ir_model': dropped constraint 'ir_model_obj_name_uniq'. Reason: its definition changed from '' to 'unique (model)'
2017-06-22 18:05:31,078 10912 DEBUG o9-mercurio openerp.models.schema: Table 'ir_model': added constraint 'ir_model_obj_name_uniq' with definition=unique (model)
2017-06-22 18:05:31,255 10912 DEBUG o9-mercurio openerp.modules.module: module database_cleanup: finalizing and (possibly) reseting blacklists for [cleanup.purge.line, cleanup.purge.wizard, ir.model.data, cleanup.purge.line.module, cleanup.purge.wizard.module, ir.model, cleanup.purge.line.model, cleanup.purge.wizard.model, cleanup.purge.line.column, cleanup.purge.wizard.column, cleanup.purge.line.table, cleanup.purge.wizard.table, cleanup.purge.line.data, cleanup.purge.wizard.data, cleanup.purge.line.menu, cleanup.purge.wizard.menu, ir.model.fields, cleanup.create_indexes.line, cleanup.create_indexes.wizard, cleanup.purge.line.property, cleanup.purge.wizard.property]

I can also see that models modified are also finalized:

2017-06-22 18:05:30,834 10912 DEBUG o9-mercurio openerp.modules.module: module auth_crypt models: [res.users]
2017-06-22 18:05:30,859 10912 INFO o9-mercurio openerp.addons.auth_crypt.auth_crypt: Hashing passwords, may be slow for databases with many users...
2017-06-22 18:05:30,860 10912 DEBUG o9-mercurio openerp.modules.module: module auth_crypt: finalizing and (possibly) reseting blacklists for [res.users]

[pedrobaeza]

What is the status of this?

[mvaled]

I think this was (somehow) merged in #663, perhaps rebased. See also 7aa365d.

[pedrobaeza]

OK, I think so also. Then I close this. If not everything is already merged, then feel free to reopen.