Skip to content

[IMP] core: populate CLI improvements - #85537

Closed
Feyensv wants to merge 2 commits into
odoo:masterfrom
odoo-dev:master-populate-profiling-vfe
Closed

[IMP] core: populate CLI improvements#85537
Feyensv wants to merge 2 commits into
odoo:masterfrom
odoo-dev:master-populate-profiling-vfe

Conversation

@Feyensv

@Feyensv Feyensv commented Feb 28, 2022

Copy link
Copy Markdown
Contributor

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo

robodoo commented Feb 28, 2022

Copy link
Copy Markdown
Contributor

Pull request status dashboard

@Feyensv
Feyensv marked this pull request as draft February 28, 2022 16:51
@C3POdoo C3POdoo added the RD research & development, internal work label Feb 28, 2022
@Feyensv
Feyensv force-pushed the master-populate-profiling-vfe branch from 3077a51 to 8637ad0 Compare March 1, 2022 16:56
@Feyensv

Feyensv commented Mar 3, 2022

Copy link
Copy Markdown
Contributor Author

@Xavier-Do Don't you think it could be useful to integrate the profiling feature directly into the populate script ? (And eventually the rollback part, but I can drop it, it's less useful).

@Xavier-Do

Xavier-Do commented Mar 7, 2022

Copy link
Copy Markdown
Contributor

Forgot my last message, I didn't look at the diff. I don't have any objections if it is opt-in :) But keep in mind that the default profiler parameters includes the sql collector witch have an impact for a lot of fast queries. It would be great to have a way to define what we want to profile. I also wonder if a --profile option isn't too generic we may want to add that to odoo-bin some day.

Those comments are just ideas the current solution looks great!

Comment thread odoo/cli/populate.py Outdated
Comment thread odoo/cli/populate.py
Comment thread odoo/cli/populate.py Outdated
@Feyensv
Feyensv force-pushed the master-populate-profiling-vfe branch from 73fe8e7 to f189453 Compare March 10, 2022 15:11
@Feyensv

Feyensv commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

But keep in mind that the default profiler parameters includes the sql collector witch have an impact for a lot of fast queries. It would be great to have a way to define what we want to profile.

As I see it, when you use the profiler, you should limit yourself to the small (/medium) population size, since you'll globally have the same results and perf bottlenecks when investigating the creation of X records instead of X*10 records. (I even think I got some strange errors while trying profiling & rollback on complex models/sizes).
I could add a choice of profilers, but I think it would be too much noise when most people always enable/use all the profilers at once.

I also wonder if a --profile option isn't too generic we may want to add that to odoo-bin some day.

I don't have a better option in mind (and --populate-profile is overkill imho).
Can't we consider this usecase when we really want to add an option in the generic command ?

@tde-banana-odoo

Copy link
Copy Markdown
Contributor

Is there any human readable summary :D ?

@Feyensv

Feyensv commented Mar 22, 2022

Copy link
Copy Markdown
Contributor Author
  1. odoo-bin populate --profile to Create ir.profile record for each model population (to have a speedscope to investigate)
  2. odoo-bin populate --rollback to avoid having to drop the db/copy a template db between two populate calls (Example, you cannot populate res.company twice on a db because of constraints on the model). With this, you could trigger the populate multiple times more easily to see the impact of code changes :).

@Feyensv
Feyensv requested a review from Xavier-Do April 26, 2022 10:56
Comment thread odoo/cli/populate.py
Comment thread odoo/cli/populate.py Outdated
Comment thread odoo/cli/populate.py Outdated
Feyensv added 2 commits May 19, 2022 14:53
Profiling: creation of ir.profile records for each models population
Rollback: allow triggering multiple times the same population,
without any risk of records conflicts (& having to recreated a clean database)

Note: those two options are mostly recommended for small models population.
Indeed, profiling slows down the population, and large/medium population is
already quite slow for some models (and it seems that the rollback breaks as well
for some advanced models).
@Feyensv
Feyensv force-pushed the master-populate-profiling-vfe branch from f189453 to 148af51 Compare May 19, 2022 13:05
@Feyensv Feyensv changed the title [POC][IMP] core: populate CLI improvements [IMP] core: populate CLI improvements May 19, 2022
@Feyensv
Feyensv marked this pull request as ready for review May 19, 2022 14:14
@Feyensv

Feyensv commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

@robodoo r+

@Feyensv

Feyensv commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

@robodoo rebase-ff

@robodoo

robodoo commented May 19, 2022

Copy link
Copy Markdown
Contributor

Merge method set to rebase and fast-forward

robodoo pushed a commit that referenced this pull request May 19, 2022
Profiling: creation of ir.profile records for each models population
Rollback: allow triggering multiple times the same population,
without any risk of records conflicts (& having to recreated a clean database)

Note: those two options are mostly recommended for small models population.
Indeed, profiling slows down the population, and large/medium population is
already quite slow for some models (and it seems that the rollback breaks as well
for some advanced models).

Part-of: #85537
robodoo pushed a commit that referenced this pull request May 19, 2022
closes #85537

Signed-off-by: Victor Feyens (vfe) <vfe@odoo.com>
@robodoo robodoo closed this May 19, 2022
@robodoo
robodoo temporarily deployed to merge May 19, 2022 17:04 Inactive
@robodoo robodoo added the 15.4 label May 19, 2022
@fw-bot
fw-bot deleted the master-populate-profiling-vfe branch June 2, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

15.4 RD research & development, internal work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants