Merge lp:~camptocamp/carriers-deliveries/7.0-threaded-dispatch-label-generation into lp:~stock-logistic-core-editors/carriers-deliveries/7.0

Proposed by Yannick Vaucher @ Camptocamp
Status: Needs review
Proposed branch: lp:~camptocamp/carriers-deliveries/7.0-threaded-dispatch-label-generation
Merge into: lp:~stock-logistic-core-editors/carriers-deliveries/7.0
Prerequisite: lp:~camptocamp/carriers-deliveries/7.0-delivery_carrier_label_dispatch-output-file-yvr
Diff against target: 230 lines (+148/-27)
2 files modified
delivery_carrier_label_dispatch/__openerp__.py (+14/-1)
delivery_carrier_label_dispatch/wizard/generate_labels.py (+134/-26)
To merge this branch: bzr merge lp:~camptocamp/carriers-deliveries/7.0-threaded-dispatch-label-generation
Reviewer Review Type Date Requested Status
Nicolas Bessi - Camptocamp (community) Approve
Stock and Logistic Core Editors Pending
Review via email: mp+215184@code.launchpad.net

Commit message

delivery_carrier_label_dispatch - Improve efficiency of label generation by threading it. For instance for postlogistics, using serialized calls to the SOAP web service is a really slow as it would wait for a response before sending the next request.

Also improve exception feed back. And thus exception are non blocking for all label generation. You will only have to relaunch generation for failed ones.

To post a comment you must log in.
42. By Yannick Vaucher @ Camptocamp on 2014-04-10

removed conflict garbage

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :
Download full text (4.4 KiB)

Hello,

Thank for the patch

Small PEP8 in manifest also noticed that
the title underline is not correct

[Link module] Carrier labels - Picking dispatch
==============================

It should go to the end of previous line

sys is imported but not used

remark
q_except can be a simpler data cohntainer like a list has they are thread safe.

the nested try catch structure here is strange

        try:
            try:
                picking_out_obj.generate_labels(
                    thread_cr, uid, [picking.id],
                    tracking_ids=tracking_ids,
                    context=context)
            except orm.except_orm as e:
                # add information on picking and pack in the exception
                picking_name = _('Picking: %s') % picking.name
                pack_num = _('Pack: %s') % pack.name if pack else ''
                raise orm.except_orm(
                    e.name,
                    _('%s %s - %s') % (picking_name, pack_num, e.value)) ...

Read more...

review: Needs Fixing
43. By Yannick Vaucher @ Camptocamp on 2014-05-14

Fix rst title in manifest

44. By Yannick Vaucher @ Camptocamp on 2014-05-14

remove unused import

45. By Yannick Vaucher @ Camptocamp on 2014-05-14

[PEP8]

46. By Yannick Vaucher @ Camptocamp on 2014-05-14

rewrite exception handling for cursor and orm exceptions on generate_labels

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Thanks for the review.

I made the fixes.

I have also rewritten the exception handling, thought it does exactly the same thing. I have to use nested exception to ensure the rollback is played in anycase. Then I want to complete informations on the orm exceptions.

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

s/thought/though

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) :
47. By Yannick Vaucher @ Camptocamp on 2014-06-13

Specify Exception class to ensure catching right exceptions and avoid catching SystemExit and KeyboardInterupt exceptions

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

fixed except: -> except Exception:

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

LGTM

review: Approve
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Note to myself

This project is now hosted on https://github.com/OCA/carriers-deliveries. Please move your proposal there if you still want to merge it once fixed. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

Unmerged revisions

47. By Yannick Vaucher @ Camptocamp on 2014-06-13

Specify Exception class to ensure catching right exceptions and avoid catching SystemExit and KeyboardInterupt exceptions

46. By Yannick Vaucher @ Camptocamp on 2014-05-14

rewrite exception handling for cursor and orm exceptions on generate_labels

45. By Yannick Vaucher @ Camptocamp on 2014-05-14

[PEP8]

44. By Yannick Vaucher @ Camptocamp on 2014-05-14

remove unused import

43. By Yannick Vaucher @ Camptocamp on 2014-05-14

Fix rst title in manifest

42. By Yannick Vaucher @ Camptocamp on 2014-04-10

removed conflict garbage

41. By Yannick Vaucher @ Camptocamp on 2014-04-10

delivery_carrier_label_dispatch - Improve efficiency of label generation by threading it. For instance for postlogistics, using serialized calls to the SOAP web service is a really slow as it would wait for a response before sending the next request.

40. By Yannick Vaucher @ Camptocamp on 2014-04-10

merge lp:~camptocamp/carriers-deliveries/7.0-delivery_carrier_label_dispatch-output-file-yvr

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'delivery_carrier_label_dispatch/__openerp__.py'
2--- delivery_carrier_label_dispatch/__openerp__.py 2014-06-13 14:52:12 +0000
3+++ delivery_carrier_label_dispatch/__openerp__.py 2014-06-13 14:52:12 +0000
4@@ -27,7 +27,7 @@
5 'depends': ['base_delivery_carrier_label', 'picking_dispatch'],
6 'description': """
7 [Link module] Carrier labels - Picking dispatch
8-==============================
9+===============================================
10
11 This module adds a wizard on picking dispatch to generate the labels
12 of the packs. The labels are merged in one PDF file.
13@@ -37,6 +37,19 @@
14
15 If you don't define your pack it will be considered a picking is a single pack.
16
17+
18+Tips
19+----
20+For picking dispatch with huge number of labels to generate your can add a
21+number of worker with ir.config_parameter `shipping_label.num_workers` to
22+parallelize the generation on multiple workers.
23+This can be really useful for exemple using PostLogistics web service to
24+gain speed.
25+
26+Be careful not to set too many worker as each one will need to instanciate
27+a cursor on database and this could generate PoolErrors.
28+A good choice would be to set it as `db_maxconn` / number_of_worker / 2
29+
30 Contributors
31 ------------
32
33
34=== modified file 'delivery_carrier_label_dispatch/wizard/generate_labels.py'
35--- delivery_carrier_label_dispatch/wizard/generate_labels.py 2014-06-13 14:52:12 +0000
36+++ delivery_carrier_label_dispatch/wizard/generate_labels.py 2014-06-13 14:52:12 +0000
37@@ -20,13 +20,20 @@
38 ##############################################################################
39 from operator import attrgetter
40 from itertools import groupby
41+import Queue
42+import threading
43+import logging
44
45+from openerp import pooler
46 from openerp.osv import orm, fields
47 from openerp.tools.translate import _
48
49 from ..pdf_utils import assemble_pdf
50
51
52+_logger = logging.getLogger(__name__)
53+
54+
55 class DeliveryCarrierLabelGenerate(orm.TransientModel):
56
57 _name = 'delivery.carrier.label.generate'
58@@ -85,37 +92,138 @@
59 return None
60 return label_obj.browse(cr, uid, label_id[0], context=context)
61
62+ def _do_generate_labels(self, cr, uid, wizard, pack, picking,
63+ label, context=None):
64+ """ Generate a label in a thread safe context
65+
66+ Here we declare a specific cursor so do not launch
67+ too many threads
68+ """
69+ picking_out_obj = self.pool['stock.picking.out']
70+ # generate the label of the pack
71+ tracking_ids = [pack.id] if pack else None
72+ # create a cursor to be thread safe
73+ thread_cr = pooler.get_db(cr.dbname).cursor()
74+ try:
75+ picking_out_obj.generate_labels(
76+ thread_cr, uid, [picking.id],
77+ tracking_ids=tracking_ids,
78+ context=context)
79+ thread_cr.commit()
80+ except Exception:
81+ thread_cr.rollback()
82+ try:
83+ raise
84+ except orm.except_orm as e:
85+ # add information on picking and pack in the exception
86+ picking_name = _('Picking: %s') % picking.name
87+ pack_num = _('Pack: %s') % pack.name if pack else ''
88+ raise orm.except_orm(
89+ e.name,
90+ _('%s %s - %s') % (picking_name, pack_num, e.value))
91+ finally:
92+ thread_cr.close()
93+
94+ def worker(self, q, q_except):
95+ """ A worker to generate labels
96+
97+ Takes data from queue q
98+
99+ And if the worker encounters errors, he will add them in
100+ q_except queue
101+ """
102+ while not q.empty():
103+ args, kwargs = q.get()
104+ try:
105+ self._do_generate_labels(*args, **kwargs)
106+ except Exception as e:
107+ q_except.put(e)
108+ finally:
109+ q.task_done()
110+
111+ def _get_num_workers(self, cr, uid, context=None):
112+ """ Get number of worker parameter for labels generation
113+
114+ Optional ir.config_parameter is `shipping_label.num_workers`
115+ """
116+ param_obj = self.pool['ir.config_parameter']
117+ num_workers = param_obj.get_param(cr, uid,
118+ 'shipping_label.num_workers')
119+ if not num_workers:
120+ return 1
121+ return int(num_workers)
122+
123 def _get_all_pdf(self, cr, uid, wizard, dispatch, context=None):
124+ q = Queue.Queue()
125+ q_except = Queue.Queue()
126+
127+ # create the tasks to generate labels
128 for pack, moves, label in self._get_packs(cr, uid, wizard, dispatch,
129 context=context):
130 if not label or wizard.generate_new_labels:
131- picking_out_obj = self.pool['stock.picking.out']
132 picking = moves[0].picking_id
133- # generate the label of the pack
134- if pack:
135- tracking_ids = [pack.id]
136- else:
137- tracking_ids = None
138- try:
139- picking_out_obj.generate_labels(
140- cr, uid, [picking.id],
141- tracking_ids=tracking_ids,
142- context=context)
143- except orm.except_orm as e:
144- picking_name = _('Picking: %s') % picking.name
145- pack_num = _('Pack: %s') % pack.name if pack else ''
146- raise orm.except_orm(
147- e.name,
148- _('%s %s - %s') % (picking_name, pack_num, e.value))
149- if pack:
150- label = self._find_pack_label(cr, uid, wizard, pack,
151- context=context)
152- else:
153- label = self._find_picking_label(cr, uid, wizard, picking,
154- context=context)
155- if not label:
156- continue # no label could be generated
157+ args = (cr, uid, wizard, pack, picking, label)
158+ kwargs = {'context': context}
159+ task = (args, kwargs)
160+ q.put(task)
161+
162+ # create few workers to parallelize label generation
163+ num_workers = self._get_num_workers(cr, uid, context=context)
164+ _logger.info('Starting %s workers to generate labels', num_workers)
165+ for i in range(num_workers):
166+ t = threading.Thread(target=self.worker, args=(q, q_except))
167+ t.daemon = True
168+ t.start()
169+
170+ # wait for all tasks to be done
171+ q.join()
172+
173+ # We will not create a partial PDF if some labels weren't
174+ # generated thus we raise catched exceptions by the workers
175+ # We will try to regroup all orm exception in one
176+ if not q_except.empty():
177+
178+ error_count = {}
179+ messages = []
180+ while not q_except.empty():
181+ e = q_except.get()
182+ if isinstance(e, orm.except_orm):
183+ if not e.name in error_count:
184+ error_count[e.name] = 1
185+ else:
186+ error_count[e.name] += 1
187+ messages.append(e.value)
188+ else:
189+ # raise other exceptions like PoolError if
190+ # too many cursor where created by workers
191+ raise e
192+ titles = []
193+ for key, v in error_count.iteritems():
194+ titles.append('%sx %s' % (v, key))
195+
196+ title = _('Errors while generating labels: ') + ' '.join(titles)
197+ message = _('Some labels couldn\'t be generated. Please correct '
198+ 'following errors and generate labels again to create '
199+ 'the ones which failed.\n\n'
200+ ) + '\n'.join(messages)
201+ raise orm.except_orm(title, message)
202+
203+ # create a new cursor to be up to date with what was created by workers
204+ join_cr = pooler.get_db(cr.dbname).cursor()
205+ for pack, moves, label in self._get_packs(join_cr, uid,
206+ wizard, dispatch,
207+ context=context):
208+ picking = moves[0].picking_id
209+ if pack:
210+ label = self._find_pack_label(join_cr, uid, wizard, pack,
211+ context=context)
212+ else:
213+ label = self._find_picking_label(join_cr, uid, wizard, picking,
214+ context=context)
215+ if not label:
216+ continue # no label could be generated
217 yield label
218+ join_cr.close()
219
220 def action_generate_labels(self, cr, uid, ids, context=None):
221 """
222@@ -128,7 +236,7 @@
223 if not this.dispatch_ids:
224 raise orm.except_orm(_('Error'), _('No picking dispatch selected'))
225
226- attachment_obj = self.pool.get('ir.attachment')
227+ attachment_obj = self.pool['ir.attachment']
228
229 for dispatch in this.dispatch_ids:
230 labels = self._get_all_pdf(cr, uid, this, dispatch,

Subscribers

People subscribed via source and target branches