-
-
Notifications
You must be signed in to change notification settings - Fork 33
[18.0][MIG] stock_orderpoint_mto_as_mts: Migration to v18.0 + IMP #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[18.0][MIG] stock_orderpoint_mto_as_mts: Migration to v18.0 + IMP #39
Conversation
e2f0413 to
cc5c911
Compare
d65e46f to
02b58a9
Compare
stock_orderpoint_mto_as_mts/tests/test_stock_warehouse_mto_as_mts.py
Outdated
Show resolved
Hide resolved
| # Update orderpoints for MTO / not MTO products that are linked to | ||
| # these categories. A category is not bound to a company, search | ||
| # products accross all companies. | ||
| _logger.debug("Search products already as MTO") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the deal with all these debug comments?
Left-over from debugging session? or do we really want to keep them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be really a lot of products under that category. Showing some progress in the debug log is convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yes, but there seems to be a lot of them. It's a bit messy, but maybe it's just me.
| if warehouses: | ||
| # Find all MTO products that changed to non MTO | ||
| _logger.debug("Search products not MTO anymore") | ||
| products_not_mto = original_mto_products.filtered_domain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as before:
- use
filtered_domainonly when you truly need it, otherwise preferfilteredfor perf - if you can avoid it, especially when dealing with a potentially large number of records, filter them with search instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtered domain is making a search limited to the records subset. I have the feeling I'm doing what you suggest as second point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtered_domain is not doing a search, it's fetching all records data and filtering in python. Maybe that's the confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh indeed, I wrongly interpreted that method. I'm replacing by a search !
| _logger.debug(f"Create orderpoint for MTO - chunck {i}") | ||
| ops = op_obj.create(op_vals_list_chunk) | ||
| # free memory usage | ||
| ops.invalidate_model() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it actually improve something?
AFAICS the only thing in the loop is op_obj.create(op_vals_list_chunk), and create is flushed immediately so there's nothing really cached.
You'd probably get better peformance not spliting at all, and letting the ORM created everything on a single query
IMO this pattern only makes sense when you're reading & writing on records:
- Reading (specially with prefetching) can easily consume lots of RAM as you keep iterating on records
- Writing (without flushing) can also do the same, on a much smaller scale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I measured is that increasing the chunk size is not making it faster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is actually to just don't split in chunks, and my point is that splitting and processing in chunks here seems pointless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the benefit it too see it's progressing in the logs :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalidate_model may still be useful is there are computed fields that are triggered after create, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but, I mean.. KISS
Anyway, not blocking over this
|
@ivantodorovich Thanks a lot for your review. I tried to improve the code. Can you drop another review before I squash the fixup commits ? |
| _logger.debug(f"Create orderpoint for MTO - chunck {i}") | ||
| ops = op_obj.create(op_vals_list_chunk) | ||
| # free memory usage | ||
| ops.invalidate_model() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but, I mean.. KISS
Anyway, not blocking over this
| if warehouses: | ||
| # Find all MTO products that changed to non MTO | ||
| _logger.debug("Search products not MTO anymore") | ||
| products_not_mto = original_mto_products.filtered_domain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtered_domain is not doing a search, it's fetching all records data and filtering in python. Maybe that's the confusion?
…as_mts_orderpoint
Currently translated at 100.0% (8 of 8 strings) Translation: stock-logistics-orderpoint-16.0/stock-logistics-orderpoint-16.0-stock_orderpoint_mto_as_mts Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-orderpoint-16-0/stock-logistics-orderpoint-16-0-stock_orderpoint_mto_as_mts/it/
Currently translated at 87.5% (7 of 8 strings) Translation: stock-logistics-orderpoint-16.0/stock-logistics-orderpoint-16.0-stock_orderpoint_mto_as_mts Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-orderpoint-16-0/stock-logistics-orderpoint-16-0-stock_orderpoint_mto_as_mts/fr/
Currently translated at 100.0% (8 of 8 strings) Translation: stock-logistics-orderpoint-16.0/stock-logistics-orderpoint-16.0-stock_orderpoint_mto_as_mts Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-orderpoint-16-0/stock-logistics-orderpoint-16-0-stock_orderpoint_mto_as_mts/tr/
8f3f260 to
da2b4fd
Compare
|
/ocabot migration stock_orderpoint_mto_as_mts |
Improve performace on each product create/write Fix the route on the category as it is propagated to children categories
da2b4fd to
6a98ae6
Compare
|
I removed the wrong .pot file |
|
/ocabot merge nobump |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 2c5bbbd. Thanks a lot for contributing to OCA. ❤️ |
Depends on:
Tested
Improvements: