odoo
[IMP] orm: parallel iter_browse
[IMP] orm: add optional parallelism to iter_browse.attr()
In some cases, e.g. if it is known that calling a certain method on the model will only trigger inserts or it is clear that updates will be disjunct, such method calls can be done in parallel.
[IMP] orm: add optional parallelism to iter_browse.create()
Like the same support added to __attr__ in the parent commit, this can only be used by callers when it is known that database modifications will be distinct, not causing concurrency issues or side-effects on the results.
create returns an iter_browse object for the caller to browse created records. To support vast amounts of created records in multiprocessing strategy, we process values in a generator and initialize the returned iter_browse object with it. As this requires the caller of create to always consume/iterate the result (otherwise records will not be created), it is not applied to the other strategies as it would break existing API.
Proposal: get rid of the hack injecting
params.Code is simpler, easier to read, and less hacky.
It is true that it looks that way, but there's one big disadvantage: it reduces the possible use-cases, because now everything in there (including {kw,}args) needs to be :cucumber:able.
Proposal: get rid of the hack injecting
params. Code is simpler, easier to read, and less hacky.It is true that it looks that way, but there's one big disadvantage: it reduces the possible use-cases, because now everything in there (including
{kw,}args) needs to be 🥒able.
Do you have a use case where we need to pass more complex objects? I think once we have it we can make this hacky.
Do you have a use case where we need to pass more complex objects? I think once we have it we can make this hacky.
Honestly, I find it more hacky to pass around all the constant parameters through IPC for each task than to store them away once before fork. I think since we're in an importable script and do not need to rename the module like in upgrade scripts, we don't have to use the attribute on the function object, if that's what is ugly. I guess, we could also just use a global here (but i would need to try it first to be sure).
Do you have a use case where we need to pass more complex objects? I think once we have it we can make this hacky.
Honestly, I find it more hacky to pass around all the constant parameters through IPC for each task than to store them away once before
fork. I think since we're in an importable script and do not need to rename the module like in upgrade scripts, we don't have to use the attribute on the function object, if that's what is ugly. I guess, we could also just use a global here (but i would need to try it first to be sure).
You are missing the point. In your analysis, which isn't wrong, you're tying the code to the internal implementation of the standard Python library. That's hacky (not to mention attaching values to functions). The interface of map allows to pass any extra parameters we need. That's the logical solution. There is zero need of extra "what's going on here", or "are we cleaning this up after?". OTOH in current implementation, we need to know and explain that it uses pickle, that we don't want to use it, that it is better because this and that... etc, to make the hacky implementation look like it is necessary. That's my point.
I take that perhaps you meant "less performant" instead of "more hacky", that I could agree. Yet the performance improvement is probably meaningless given the time the rest would take.
EDIT: To be clear: I'm OK with current implementation. It can be r+ed once all details are tested.
I just don't like hacky stuff if there is a clearer way to express something. I agree that sometimes we need to sacrifice "clarity" for the sake of higher goals. I don't see them here.
you're tying the code to the internal implementation of the standard Python library.
What do you mean? The fact that it forks? This whole thing is tied to that, as are all of our other uses of ProcessPoolExecutor, that's why https://github.com/odoo/upgrade-util/pull/320#discussion_r2348147163 is important.
I also wanted simplicity: in the callback signature, to avoid as many serialisation issues as we can. OTOH, your suggestion is more readable and easier to understand. I think both are valid goals and they are only in conflict here, because ProcessPoolExecutor.map (not map) is not a simple interface, it just pretends to be. I made this point on the last PR already and it didn't change, there are all kinds of quirks in this thing.
Anyway. It is true that input to this code - if used - has to be chosen very carefully for all kinds of reasons. So maybe it is a good idea to restrict, in this way, the arguments that can be passed to the model attr being called, for the purpose of limiting the use-cases to simpler ones that hopefully have a lower risk of dangerous use.
What do you mean? The fact that it forks?
No, the fact that map would use pickle to send the parameters.
Once more, I'm OK with this. You could probably include some comments about why you chose to do this. I'd say something like: "we want to avoid the overhead of serializing the same parameters for all ids via pickle in the map implementation. Warning: we need to clean them up in _end to avoid any potential conflicts".
Note that we need to keep in mind --some testing would be nice-- what happens for multiple calls to iter_browse with multiprocessing within the same run (upgrade step). I don't see anything problematic at the moment.
it just pretends to be
Yes. We should honor that --if we can--, that's the point.
In order to move on with this PR, let split it. Can you please extract the "generator and query" commits into a new PR?
In order to move on with this PR, let split it. Can you please extract the "generator and query" commits into a new PR?
created https://github.com/odoo/upgrade-util/pull/350 with those two commits