[trunk] many2one fields should be read as (id, name) also for osv_memory objects (saving name_get RPC calls)

[Raphaël Valyi - http://www.akretion.com]

Hello,

I've a production case with 315 moves in a picking, when clicking on process, the wizard last for nearly a minute to appear depending on your network latency because it makes a name_get RPC call for each line instead of reading it by batch, see the following GTK RPC logs + full description here (I open a new bug as those are separated concerns): https://bugs.launchpad.net/openobject-addons/+bug/709559

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

I don't understand why you flagged my 2 other reports as duplicate. Again, that sounds just an inconsistent policy.
In the past, we have always been told to open new bugs for different concerns, I really see 3 concerns here:
1) missing records due to harcoded limit being reached silently
2) useless + slow name_get RPC calls from the GTK client (this one)
3) server slowness.

Regards

[Olivier Dony (Odoo) (odo-openerp)]

You're right, this is a separate issue, but more an optimization, with a low priority, could even be a wishlist.

[filsys (office-filsystem)]

@Olivier, just a comment:
I think that 'low priority' is not adequated priority for community. In our production sever we have:
58 locations,
2800 products with tracking lots on input and output,
approx. 50 PO with associated invoices per day,
approx 500 SO with associated invoices per day,
8 shops with price included and POS installed and 8 cash registers,
8 internal pickings for move from warehouse to shop locations.
In our localization we create account move for every stock move in, out, internal and consume.
Our picking have max 75 items, but when process picking in and picking out, we wait for tens of seconds even in internal network.
So, please set priority more high.
Thanks.

[Olivier Dony (Odoo) (odo-openerp)]

@filsys: I understand your concern, but the current bug report is just about the way the GTK client calls the name_get method on o2m lines, the original bug report was split into separate bugs that have a greater impact on the stock move processing and have been set to higher priorities (bug 555271 and bug 709575). BTW the importance of a bug is not directly linked to the resolution time.

[filsys (office-filsystem)]

"BTW the importance of a bug is not directly linked to the resolution time."
It's ok, but I've supposed that importance of a bug is directly linked to the time when the bug is assigned to a team.
Thanks.

[Naresh(OpenERP) (nch-openerp)]

Hello,

Regarding the 'name_get' call:

The architecture that the GTK as well as web follows is that for each O2M (here product_moves) it will perform a read now say it returned a list of dictionary([{}]) of 9 records eg: [{'product_id': 3, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 50, 'quantity': 1.0}, {'product_id': 9, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 51, 'quantity': 1.0}, {'product_id': 16, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 52, 'quantity': 1.0}, {'product_id': 13, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 53, 'quantity': 1.0}, {'product_id': 12, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 54, 'quantity': 1.0}, {'product_id': 11, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 55, 'quantity': 1.0}, {'product_id': 14, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 56, 'quantity': 1.0}, {'product_id': 21, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 57, 'quantity': 1.0}, {'product_id': 3, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 58, 'quantity': 3.0}]

Now the GTK and web will iterate over this list and then internally by field so based on the type of the field it will create the instance of that type of field say for eg:here we take 'product_id' now it will make a call to 'name_get' get its name and fill the many2one field. similarly for the rest of the records.

The same architecture is implemented in GTK and web.

@Raphaël Valyi
 The screenshot that you provided in comment #2 @lp:709559 . The wizard will only show you the moves that are still left to be processed.

Thanks

[Olivier Dony (Odoo) (odo-openerp)]

After Naresh's investigation, it appears this issue stems from a server-side shortcoming: read() on many2one fields in osv_memory objects did not return the name, but only the ID of the target record.
Clients expect read() to return a tuple with (id, name) for many2one records, otherwise they have to manually call name_get() in an "out-of-band" fashion, which cannot be easily batched.

This will be fixed in server soon, and will improve the situation for all wizards and all clients at once.

[Olivier Dony (Odoo) (odo-openerp)]

The fix for this has landed in the following server revisions:
trunk: 3338 <email address hidden>
6.0: 3330 <email address hidden>

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

@Naresh,

Thank you for investigating, I believe the analysis of Olivier is exact
about the read in osv_mem.
about your comment "The screenshot that you provided in comment #2
@lp:709559 . The wizard will only show you the moves that are still left to
be processed."
-> the difference of stock move numbers in the popup really comes from an
OpenERP bug (aknowledged now) because of some hard limit in the number of
osv_memory records OpenERP sessions can hold currently. But this was a
different issue and is dealt in a separated bug report.

On Tue, Feb 1, 2011 at 6:59 AM, Naresh(OpenERP) <email address hidden> wrote:

> Hello,
>
> Regarding the 'name_get' call:
>
> The architecture that the GTK as well as web follows is that for each
> O2M (here product_moves) it will perform a read now say it returned a
> list of dictionary([{}]) of 9 records eg: [{'product_id': 3,
> 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 50,
> 'quantity': 1.0}, {'product_id': 9, 'product_uom': 1, '__last_update':
> False, 'prodlot_id': False, 'id': 51, 'quantity': 1.0}, {'product_id':
> 16, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id':
> 52, 'quantity': 1.0}, {'product_id': 13, 'product_uom': 1,
> '__last_update': False, 'prodlot_id': False, 'id': 53, 'quantity': 1.0},
> {'product_id': 12, 'product_uom': 1, '__last_update': False,
> 'prodlot_id': False, 'id': 54, 'quantity': 1.0}, {'product_id': 11,
> 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id': 55,
> 'quantity': 1.0}, {'product_id': 14, 'product_uom': 1, '__last_update':
> False, 'prodlot_id': False, 'id': 56, 'quantity': 1.0}, {'product_id':
> 21, 'product_uom': 1, '__last_update': False, 'prodlot_id': False, 'id':
> 57, 'quantity': 1.0}, {'product_id': 3, 'product_uom': 1,
> '__last_update': False, 'prodlot_id': False, 'id': 58, 'quantity': 3.0}]
>
> Now the GTK and web will iterate over this list and then internally by
> field so based on the type of the field it will create the instance of
> that type of field say for eg:here we take 'product_id' now it will make
> a call to 'name_get' get its name and fill the many2one field. similarly
> for the rest of the records.
>
> The same architecture is implemented in GTK and web.
>
> @Raphaël Valyi
> The screenshot that you provided in comment #2 @lp:709559 . The wizard
> will only show you the moves that are still left to be processed.
>
> Thanks
>
> --
> You received this bug notification because you are a direct subscriber
> of the bug.
> https://bugs.launchpad.net/bugs/709567
>
> Title:
> [6.0.1][stock] large picking slow performance
>
> Status in OpenERP GTK Client:
> Confirmed
>
> Bug description:
> Hello,
>
> I've a production case with 315 moves in a picking, when clicking on
> process, the wizard last for nearly a minute to appear depending on
> your network latency because it makes a name_get RPC call for each
> line instead of reading it by batch, see the following GTK RPC logs +
> full description here (I open a new bug as those are separated
> concerns): https://bugs.launchpad.net/openobject-addons/+bug/709559
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/openob...

[Olivier Dony (Odoo) (odo-openerp)]

The fix in 6.0 was reverted while we investigate if all addons are properly compatible with the corrected behavior... keeping bug open.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello Olivier,

it doesn't seem reverted on trunk. Still, I can confirm, that we could appreciate the noticeable perf boost for the picking popup in prod already. Thanks. Crossing fingers for the compatibility.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Yep indeed, we found the first API change impact in prod this afternoon
while exporting electronic invoices with the Brazilian localization...
I suggest you test deeply indeed. Sad to find such API discrepancy after the
release, but I still suggest to fix it rather than wait crappy code to
develop on the top of the broken API + slow perf.

On Tue, Feb 1, 2011 at 12:05 PM, Raphaël Valyi - http://www.akretion.com <
<email address hidden>> wrote:

> Hello Olivier,
>
> it doesn't seem reverted on trunk. Still, I can confirm, that we could
> appreciate the noticeable perf boost for the picking popup in prod
> already. Thanks. Crossing fingers for the compatibility.
>
> --
> You received this bug notification because you are a direct subscriber
> of the bug.
> https://bugs.launchpad.net/bugs/709567
>
> Title:
> [6.0.1] many2one fields should be read as (id, name) also for
> osv_memory objects (saving name_get RPC calls)
>
> Status in OpenERP Server:
> In Progress
>
> Bug description:
> Hello,
>
> I've a production case with 315 moves in a picking, when clicking on
> process, the wizard last for nearly a minute to appear depending on
> your network latency because it makes a name_get RPC call for each
> line instead of reading it by batch, see the following GTK RPC logs +
> full description here (I open a new bug as those are separated
> concerns): https://bugs.launchpad.net/openobject-addons/+bug/709559
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/openobject-server/+bug/709567/+subscribe
>

[Olivier Dony (Odoo) (odo-openerp)]

Reverted in trunk too, we initially planned to fix trunk wizards quickly, but this is taking too long, so I'm reverting it in trunk too, sorry. Work in progress for fixing the wizards can be found here: lp:~openerp/openobject-addons/trunk-fix-osvmem-read

I would definitely vote for fixing the discrepancy at this stage too, but the consequences are huge... almost every other wizard needs a patch, which makes the risk important in stable-6.0... :-(

[Olivier Dony (Odoo) (odo-openerp)]

BTW you can fix your own modules already by making sure to use browse() instead of read() as much as possible, and if read() is really more suited, test whether the value is a tuple and replace it with its first element in that case. It won't hurt in any case, even without the change.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Olivier,

fixing the Brazilian localization to cope to the API change was very easy
indeed. The only dilemna I have is:
1) should I accept the bad perf and use the trunk version
2) should I try the API fix at the risk of having more wizard bugs...
This is a hard question. I hope OpenERP SA will go for a fix, we all know
that waiting is only making the issue much bigger at the next release change
at this stage. Because inconsistent API mean people will write tons of hack
to make it work no matter what, but this is never going to become clean
again, this is an entropy matter...

On Tue, Feb 1, 2011 at 7:13 PM, Olivier Dony (OpenERP) <
<email address hidden>> wrote:

> BTW you can fix your own modules already by making sure to use browse()
> instead of read() as much as possible, and if read() is really more
> suited, test whether the value is a tuple and replace it with its first
> element in that case. It won't hurt in any case, even without the
> change.
>
> --
> You received this bug notification because you are a direct subscriber
> of the bug.
> https://bugs.launchpad.net/bugs/709567
>
> Title:
> [6.0.1] many2one fields should be read as (id, name) also for
> osv_memory objects (saving name_get RPC calls)
>
> Status in OpenERP Server:
> In Progress
>
> Bug description:
> Hello,
>
> I've a production case with 315 moves in a picking, when clicking on
> process, the wizard last for nearly a minute to appear depending on
> your network latency because it makes a name_get RPC call for each
> line instead of reading it by batch, see the following GTK RPC logs +
> full description here (I open a new bug as those are separated
> concerns): https://bugs.launchpad.net/openobject-addons/+bug/709559
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/openobject-server/+bug/709567/+subscribe
>

[Olivier Dony (Odoo) (odo-openerp)]

Here is a very short sample of timing the various parts of the operations, performed on a local server with the latest trunk (hopefully LP preserves the formatting):

| # | A | B | C | D |
| 64 | 7 | 5 | 2 | 15 |
| 128 | 14 | 11 | 3 | 56 |
| 256 | 28 | 21 | 4 | 232 |

with:
#: lines in purchase.order
A: time to confirm po (seconds)
B: time to duplicate po (seconds)
C: time to open picking processing wizard (seconds)
D: time to process picking with wizard (seconds)

This not only confirms that step D has a total time in O(n2), which is too much (to be fixed by bug 709575), but also shows that the name_get() calls from the client, counted in column C, are almost negligible, even if YMMV depending on the network speed (this is the current bug).

Given that we are already considering extreme cases here, with a wizard that include hundreds of m2o values, the performance improvement of fixing the current bug in 6.0 seems totally irrelevant compared to the huge impact of changing the API at this stage.
This change is likely to require patching 80% of osv_memory wizards when going from 6.0.1 to 6.0.2, i.e. a minor/patch version update! And not only for core addons, but all community addons too. This is against all stable policies. Imagine the regression risk, even with yaml tests in place and careful manual check.

As a conclusion, we won't change this in 6.0, but will do it as soon as possible in trunk. Progress can be followed in the work-in-progress branches:
server: lp:~openerp-dev/openobject-server/trunk-fix-osvmem-read
addons: lp:~openerp/openobject-addons/trunk-fix-osvmem-read

PS: if you still want to experience the small perf boost in 6.0 without the global impact of the API change, you can try this server 6.0 branch, which contains a special 'osv_memory_v610' class: lp:~openerp-dev/openobject-server/6.0-osv-mem-v610. Then make whatever wizard you want (e.g stock.move.memory.out) inherit osv_memory_v610 instead of osv_memory, and you will have the same benefits. But the difference isn't probably worth it...

[Olivier Dony (Odoo) (odo-openerp)]

This has finally been fixed in trunk, after updating the addons to the new API:

server-trunk: revision 3369 <email address hidden>
addons-trunk: after revision 4542 <email address hidden>

This API change will require updating custom/extra addons that contain osv_memory wizards, especially if they are using or returning m2o field values.

For more details, please see this post on the framework mailing-list: https://lists.launchpad.net/openerp-expert-framework/msg00525.html