Flaky errors using "itertools.groupby" returning different result based on order so loosing data - 13.0, 14.0, 15.0 #105376

moylop260 · 2022-11-08T18:47:04Z

Impacted versions

Tested in 15.0 but the same buggy code

odoo/addons/pos_restaurant/models/pos_order.py

Line 96 in bd366a6

for order_id, order_lines in groupby(extended_order_lines, key=lambda x:x[2]['order_id']):

Found for older versions

13.0

odoo/addons/pos_restaurant/models/pos_order.py

Line 84 in 4344d39

for order_id, order_lines in groupby(extended_order_lines, key=lambda x:x[2]['order_id']):

14.0

odoo/addons/pos_restaurant/models/pos_order.py

Line 91 in c1bfba9

for order_id, order_lines in groupby(extended_order_lines, key=lambda x:x[2]['order_id']):

Steps to reproduce

Check the following code and output

from itertools import groupby


extended_order_lines = [
    [0, 0, {"order_id": (16433, "/"), "qty": 1.0}],
    [0, 0, {"order_id": (16434, "/"), "qty": 1.0}],
    [0, 0, {"order_id": (16433, "/"), "qty": 1.0}],
    [0, 0, {"order_id": (16434, "/"), "qty": 1.0}],
]

extended_order_lines_sorted = sorted(extended_order_lines, key=lambda x:x[2]['order_id'][0])

groupby_res = groupby(extended_order_lines, key=lambda x:x[2]['order_id'][0])
groupby_res_list = [(order_id, len(list(order_lines))) for order_id, order_lines in groupby_res]
print(groupby_res_list)
# The output is: [(16433, 1), (16434, 1), (16433, 1), (16434, 1)]

groupby_res_sorted = groupby(extended_order_lines_sorted, key=lambda x:x[2]['order_id'][0])
groupby_res_list_sorted = [(order_id, len(list(order_lines))) for order_id, order_lines in groupby_res_sorted]
print(groupby_res_list_sorted)
# The output is: [(16433, 2), (16434, 2)]

Current behavior

Notice using the same data but changing the order the output is different

Notice the first unordered output:

[(16433, 1), (16434, 1), (16433, 1), (16434, 1)]

Neither the id 16433 was not grouped nor the id 16434 (in fact the same key is duplicated)

It is expected for python since the documentation says:

Generally, the iterable needs to already be sorted on the same key function.

But it is not expected for odoo where it is used.

The code above is based on:

odoo/addons/pos_restaurant/models/pos_order.py

Lines 94 to 95 in 230f1b4

    
           for order_id, order_lines in groupby(extended_order_lines, key=lambda x:x[2]['order_id']): 
        
               next(order for order in orders if order['id'] == order_id[0])['lines'] = list(order_lines)

Analyzing these lines of code:

for order_id, order_lines in groupby(extended_order_lines, key=lambda x:x[2]['order_id']):
    next(order for order in orders if order['id'] == order_id[0])['lines'] = list(order_lines)

If the extended_order_lines are not ordered with the key (or maybe yes, depending on your luck) so the result will be missing lines since it is assigning order_lines for the first item found for each order_id so the second item is ignored

Raising flaky errors losing data

Expected behavior

No loose data

No odoo logic calling groupby without sorting consideration

We can fix it in different ways:

Fixing current odoo state

We can generate a PR to Odoo for all the lines of code in odoo using this way, to make sure a sorted based on key generator is sent to groupby method

It is the more logical solution in the short term but what about if a future developer does the same wrong way

Deprecate iterators.groupby

We can deprecateiterators.groupby method from lint here:

odoo/odoo/addons/test_lint/tests/test_pylint.py

Lines 38 to 40 in 87698d9

BAD_FUNCTIONS = [

'input',

]

In pro to use odoo.tools.groupby method:

odoo/odoo/tools/misc.py

Lines 1168 to 1173 in bd366a6

    
           def groupby(iterable, key=None): 
        
               """ Return a collection of pairs ``(key, elements)`` from ``iterable``. The 
        
                   ``key`` is a function computing a key value for each element. This 
        
                   function is similar to ``itertools.groupby``, but aggregates all 
        
                   elements under the same key, not only consecutive elements. 
        
               """

Other solutions?
I have created this issue in order to discuss the best way to fix this flaky error in a global and standard way in order to avoid reproducing it in the future again and again for other pieces of code

It was so hard for us to detect, reproduced then justify the fix

Using an OPW ticket the team will request us to reproduce in runbot functionally but it is not easy since it is hard to reproduce because it is flaky

But technically is easier to realize where the issue is

We have customers losing data (e.g. pos order lines) because of this

The places calling itertools.groupby methods:

grep -rn " groupby(\|\.groupby(" odoo enterprise --include="*.py" --exclude-dir="test*" |grep -v "E\.groupby"

UPDATED: I just found a blog about this matter:

https://sharats.me/posts/python-itertools-groupby-callable/

The text was updated successfully, but these errors were encountered:

moylop260 · 2022-11-08T19:31:29Z

Hi @odony

I'm curious about your expert opinion here

Thanks in advance!

More info about odoo/odoo#105376

odony · 2022-11-24T17:48:35Z

@moylop260 thanks for the nice analysis and the simple repro/demo code! :-)

I guess there's no shortcut, the good solution is to review all the cases where we've used itertools.groupby and fix them to user odoo.tool.groupby, which should be a drop-in replacement.

Then we could indeed discourage its usage with a pylint test. We can make it specifically for itertools.groupby, not other groupby calls. (/cc @flvr-odoo who is working on pylint lately ;-))

@rco-odoo any thoughts here?

moylop260 · 2022-11-24T18:30:03Z

Hi @odony

I can do the PR to test_lint if you agree

FYI I have already created a static lint for this here:

[ADD] bad-builtin-groupby: Prefer odoo.tools.groupby instead OCA/pylint-odoo#442

(But for odoo it could be a dynamic one since that it is running from an module with all the dependencies installed)

Let me know if you agree

moylop260 · 2022-11-24T20:12:09Z

@odony @flvr-odoo

Please, be aware about the following lint PR:

[ADD] test_lint: New check to prefer odoo.tools.groupby #106478

….tools.groupby See odoo#105376

…by key More info odoo#105376

voronind · 2022-12-23T02:39:19Z

There is no error. groupby works correctly

moylop260 changed the title ~~Flaky errors using "itertools.groupby" returning different result based on order~~ Flaky errors using "itertools.groupby" returning different result based on order so loosing data Nov 8, 2022

moylop260 changed the title ~~Flaky errors using "itertools.groupby" returning different result based on order so loosing data~~ Flaky errors using "itertools.groupby" returning different result based on order so loosing data - 13.0, 14.0, 15.0 Nov 8, 2022

fernandahf mentioned this issue Nov 8, 2022

[15.0][FIX] pos_restaurant: use groupby of odoo instead itertools #105388

Closed

moylop260 added a commit to vauxoo-dev/pylint-odoo that referenced this issue Nov 9, 2022

[ADD] bad-builtin-groupby: Prefer odoo.tools.groupby instead

8d6616c

More info about odoo/odoo#105376

moylop260 mentioned this issue Nov 9, 2022

[ADD] bad-builtin-groupby: Prefer odoo.tools.groupby instead OCA/pylint-odoo#442

Merged

moylop260 added a commit to vauxoo-dev/pylint-odoo that referenced this issue Nov 9, 2022

[ADD] bad-builtin-groupby: Prefer odoo.tools.groupby instead

4f2cee8

More info about odoo/odoo#105376

moylop260 added a commit to vauxoo-dev/pylint-odoo that referenced this issue Nov 9, 2022

[ADD] bad-builtin-groupby: Prefer odoo.tools.groupby instead

d7f9104

More info about odoo/odoo#105376

moylop260 added a commit to OCA/pylint-odoo that referenced this issue Nov 9, 2022

[ADD] bad-builtin-groupby: Prefer odoo.tools.groupby instead (#442)

167ff09

More info about odoo/odoo#105376

fernandahf mentioned this issue Nov 9, 2022

[14.0][FIX] pos_restaurant: use groupby of odoo instead itertools #105518

Closed

fw-bot mentioned this issue Nov 24, 2022

[FW][14.0][FIX] pos_restaurant: use groupby of odoo instead itertools #106475

Closed

moylop260 mentioned this issue Nov 24, 2022

[ADD] test_lint: New check to prefer odoo.tools.groupby #106478

Closed

moylop260 added a commit to vauxoo-dev/odoo that referenced this issue Dec 15, 2022

[ADD] test_lint: prefer-odoo-tools-groupby - New check to prefer odoo…

adecaf3

….tools.groupby See odoo#105376

moylop260 added a commit to vauxoo-dev/odoo that referenced this issue Dec 15, 2022

[REF] *: Use odoo.tools.groupby instead of itertools.groupby or sort …

547a5bc

…by key More info odoo#105376

antonag32 mentioned this issue Oct 2, 2023

[REF] Filter out "bump version" from CHANGELOG OCA/pylint-odoo#472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flaky errors using "itertools.groupby" returning different result based on order so loosing data - 13.0, 14.0, 15.0 #105376

Flaky errors using "itertools.groupby" returning different result based on order so loosing data - 13.0, 14.0, 15.0 #105376

moylop260 commented Nov 8, 2022 •

edited

Loading

moylop260 commented Nov 8, 2022 •

edited

Loading

odony commented Nov 24, 2022

moylop260 commented Nov 24, 2022

moylop260 commented Nov 24, 2022

voronind commented Dec 23, 2022

Flaky errors using "itertools.groupby" returning different result based on order so loosing data - 13.0, 14.0, 15.0 #105376

Flaky errors using "itertools.groupby" returning different result based on order so loosing data - 13.0, 14.0, 15.0 #105376

Comments

moylop260 commented Nov 8, 2022 • edited Loading

Impacted versions

Steps to reproduce

Current behavior

Expected behavior

moylop260 commented Nov 8, 2022 • edited Loading

odony commented Nov 24, 2022

moylop260 commented Nov 24, 2022

moylop260 commented Nov 24, 2022

voronind commented Dec 23, 2022

moylop260 commented Nov 8, 2022 •

edited

Loading

moylop260 commented Nov 8, 2022 •

edited

Loading