Merge lp:~numerigraphe-team/ocb-addons/7.0-fill-inventory-OOM into lp:ocb-addons

Proposed by Loïc Bellier - Numérigraphe
Status: Merged
Merged at revision: 10210
Proposed branch: lp:~numerigraphe-team/ocb-addons/7.0-fill-inventory-OOM
Merge into: lp:ocb-addons
Diff against target: 66 lines (+27/-20)
1 file modified
stock/wizard/stock_fill_inventory.py (+27/-20)
To merge this branch: bzr merge lp:~numerigraphe-team/ocb-addons/7.0-fill-inventory-OOM
Reviewer Review Type Date Requested Status
Laurent Mignon (Acsone) (community) code review Approve
Guewen Baconnier @ Camptocamp code review, test Approve
Lionel Sausin - Initiatives/Numérigraphe (community) co-author Abstain
Holger Brunn (Therp) code review Approve
Review via email: mp+217049@code.launchpad.net

Description of the change

We're having problems importing an inventory for a location that contains >630000 Stock Moves.
The problem seems to lie in the python loop based on the browse() of the Stock Moves: it preloads too much data and pushes the OS to an out-of-memory condition (which Linux responds to by killing the openerp server).
This patch solve this bug by splitting stock moves lines ids on separated list of 10 000 lines

Run GREEN on runbot.

pull request on standard 7.0: https://github.com/odoo/odoo/pull/341

To post a comment you must log in.
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) :
review: Approve
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

I think you'd be better off using search(..., limit=your_limit, offset=your_offset).

If you don't want to make that change, at least the 10000 should be in a variable and you should use range(start, end, step)

review: Needs Fixing (code review)
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

Holger, using search with an offset is cooler when you expect to never load the end of the list (like displaying it for users).
In the case here, it would only trade python-server memory consumption for SQL round-trips. Is it worth?

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

You're right, it's not. But still I suggest doing the stepping part not by hand but as a parameter for range()

10135. By Numérigraphe

[REF] cleaner looping code

Revision history for this message
Holger Brunn (Therp) (hbrunn) :
review: Approve (code review)
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

Thanks Holger, I've updated Loïc's patch according to your suggestion.
Of course I'll abstain now that I'm co-author :)

review: Abstain (co-author)
10136. By Numérigraphe

[DEL] remove unused import

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

LGTM.

Thanks!

review: Approve (code review, test)
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

Thank you. For the archives, the MP on official addons was turned into a pull request on github here: https://github.com/odoo/odoo/pull/341

Revision history for this message
Laurent Mignon (Acsone) (lmi) wrote :

Thanks for the MP.

LGTM for V7

Note that for V8, with the new API (https://github.com/odoo-dev/odoo/blob/master-apiculture/openerp/osv/orm.py), the cache is managed in an other way and is no more reset on each call to the browse method.

lmi

review: Approve (code review)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'stock/wizard/stock_fill_inventory.py'
2--- stock/wizard/stock_fill_inventory.py 2014-03-24 09:14:18 +0000
3+++ stock/wizard/stock_fill_inventory.py 2014-05-12 08:28:17 +0000
4@@ -27,6 +27,9 @@
5 _name = "stock.fill.inventory"
6 _description = "Import Inventory"
7
8+ # Maximum size of a batch of lines we can import without risking OOM
9+ MAX_IMPORT_LINES = 10000
10+
11 def _default_location(self, cr, uid, ids, context=None):
12 try:
13 location = self.pool.get('ir.model.data').get_object(cr, uid, 'stock', 'stock_location_stock')
14@@ -103,28 +106,32 @@
15 for location in location_ids:
16 datas = {}
17 res[location] = {}
18- move_ids = move_obj.search(cr, uid, ['|',('location_dest_id','=',location),('location_id','=',location),('state','=','done')], context=context)
19+ all_move_ids = move_obj.search(cr, uid, ['|',('location_dest_id','=',location),('location_id','=',location),('state','=','done')], context=context)
20 local_context = dict(context)
21 local_context['raise-exception'] = False
22- for move in move_obj.browse(cr, uid, move_ids, context=context):
23- lot_id = move.prodlot_id.id
24- prod_id = move.product_id.id
25- if move.location_dest_id.id != move.location_id.id:
26- if move.location_dest_id.id == location:
27- qty = uom_obj._compute_qty_obj(cr, uid, move.product_uom,move.product_qty, move.product_id.uom_id, context=local_context)
28- else:
29- qty = -uom_obj._compute_qty_obj(cr, uid, move.product_uom,move.product_qty, move.product_id.uom_id, context=local_context)
30-
31-
32- if datas.get((prod_id, lot_id)):
33- qty += datas[(prod_id, lot_id)]['product_qty']
34-
35- # Floating point sum could introduce tiny rounding errors :
36- # Use the UoM API for the rounding (same UoM in & out).
37- qty = uom_obj._compute_qty_obj(cr, uid,
38- move.product_id.uom_id, qty,
39- move.product_id.uom_id)
40- datas[(prod_id, lot_id)] = {'product_id': prod_id, 'location_id': location, 'product_qty': qty, 'product_uom': move.product_id.uom_id.id, 'prod_lot_id': lot_id}
41+ # To avoid running out of memory, process limited batches
42+ for i in range(0, len(all_move_ids), self.MAX_IMPORT_LINES):
43+ move_ids = all_move_ids[i * self.MAX_IMPORT_LINES:
44+ (i + 1) * self.MAX_IMPORT_LINES]
45+ for move in move_obj.browse(cr, uid, move_ids, context=context):
46+ lot_id = move.prodlot_id.id
47+ prod_id = move.product_id.id
48+ if move.location_dest_id.id != move.location_id.id:
49+ if move.location_dest_id.id == location:
50+ qty = uom_obj._compute_qty_obj(cr, uid, move.product_uom,move.product_qty, move.product_id.uom_id, context=local_context)
51+ else:
52+ qty = -uom_obj._compute_qty_obj(cr, uid, move.product_uom,move.product_qty, move.product_id.uom_id, context=local_context)
53+
54+
55+ if datas.get((prod_id, lot_id)):
56+ qty += datas[(prod_id, lot_id)]['product_qty']
57+
58+ # Floating point sum could introduce tiny rounding errors :
59+ # Use the UoM API for the rounding (same UoM in & out).
60+ qty = uom_obj._compute_qty_obj(cr, uid,
61+ move.product_id.uom_id, qty,
62+ move.product_id.uom_id)
63+ datas[(prod_id, lot_id)] = {'product_id': prod_id, 'location_id': location, 'product_qty': qty, 'product_uom': move.product_id.uom_id.id, 'prod_lot_id': lot_id}
64
65 if datas:
66 flag = True