Skip to content
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

Optimize test statistic queries #7789

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

martonmiklos
Copy link
Contributor

@martonmiklos martonmiklos commented Aug 1, 2024

Replace individual queries with joins in the test statistic queries

Copy link

netlify bot commented Aug 1, 2024

Deploy Preview for inventree-web-pui-preview ready!

Name Link
🔨 Latest commit 1a09ef0
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/66acc5ebb7267300087feaf2
😎 Deploy Preview https://deploy-preview-7789--inventree-web-pui-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 86 (no change from production)
Best Practices: 92 (no change from production)
SEO: 70 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@martonmiklos martonmiklos force-pushed the optimize_test_statistic_queries branch from 5ba8743 to de2a067 Compare August 1, 2024 21:21
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.96%. Comparing base (6fd5a99) to head (1a09ef0).
Report is 358 commits behind head on master.

Files with missing lines Patch % Lines
src/backend/InvenTree/stock/models.py 53.33% 7 Missing ⚠️
src/backend/InvenTree/stock/serializers.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7789      +/-   ##
==========================================
- Coverage   83.49%   82.96%   -0.53%     
==========================================
  Files        1121     1119       -2     
  Lines       49829    50751     +922     
  Branches     1645     1618      -27     
==========================================
+ Hits        41606    42107     +501     
- Misses       7781     8212     +431     
+ Partials      442      432      -10     
Flag Coverage Δ
backend 85.34% <87.09%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SchrodingersGat
Copy link
Member

@martonmiklos for some reason when testing this I am getting a 404 error which I did not seem to be getting before:

image

This is using part 903 on the demo dataset

image

Note that I also get the same 404 on master branch - so it is not related to this PR.

Note also that the "by-build" API endpoint seems to function correctly still:

image

@martonmiklos
Copy link
Contributor Author

martonmiklos commented Aug 2, 2024

@martonmiklos for some reason when testing this I am getting a 404 error which I did not seem to be getting before:

image

This is using part 903 on the demo dataset

Note that I also get the same 404 on master branch - so it is not related to this PR.

Note also that the "by-build" API endpoint seems to function correctly still:

I will look into it, I kept the PR in WIP because I want to add API tests.

@martonmiklos martonmiklos changed the title [WIP]Optimize test statistic queries Optimize test statistic queries Aug 2, 2024
@martonmiklos
Copy link
Contributor Author

@martonmiklos for some reason when testing this I am getting a 404 error which I did not seem to be getting before:

Well I have to admit that I seriously messed it up here:

https://github.com/inventree/InvenTree/pull/7789/files#diff-0f0aa474fda00aebc321caa9ea7f5e241333dcee07579fad73074ed6e559663bL1323

My django knowledge appraching zero from the negative side, I would be glad if you could check whether my approach is proper here too:
https://github.com/inventree/InvenTree/pull/7789/files#diff-d4349f553750c97015938a8c71f6b6a6e6d285bd1855b35454b10d4b99c08a08L888

@SchrodingersGat SchrodingersGat added this to the 0.16.0 milestone Aug 7, 2024
@SchrodingersGat SchrodingersGat added api Relates to the API backport Apply this label to a PR to enable auto-backport action refactor backport-to-0.16.x labels Aug 7, 2024
@matmair
Copy link
Member

matmair commented Aug 9, 2024

@martonmiklos can you be bit more specific what you are trying to do in the linked row? Maybe comment on the code in the PR

@martonmiklos
Copy link
Contributor Author

@martonmiklos can you be bit more specific what you are trying to do in the linked row? Maybe comment on the code in the PR

Could you please link the row?

@matmair
Copy link
Member

matmair commented Aug 9, 2024

Could you link what specifically you want to be reviewed (what are your concerns) in which code section?

@SchrodingersGat SchrodingersGat modified the milestones: 0.16.0, 0.17.0 Aug 10, 2024
@martonmiklos
Copy link
Contributor Author

Could you link what specifically you want to be reviewed (what are your concerns) in which code section?

There are two things: the part statistic queries accumulated the pass-fail counts by iterating over the affected build outputs. This had been optimized by using filters which translates to joins in the SQL.

The other issue what had been highlighted by was higlighted here with accessing the serializer here:
https://github.com/inventree/InvenTree/pull/7789/files#diff-0f0aa474fda00aebc321caa9ea7f5e241333dcee07579fad73074ed6e559663bL1323

@SchrodingersGat
Copy link
Member

@martonmiklos now that the new user interface supports proper plugins (you can see an example here) I would like to consider whether the "test statistics" could be offloaded to a plugin. In my opinion it is not quite a "core" feature but would certainly be useful to some users!

If you are open to this, I can help set it up as a plugin, and dedicate some time to looking into what is going on with the query efficencies here...

@martonmiklos
Copy link
Contributor Author

@martonmiklos now that the new user interface supports proper plugins (you can see an example here) I would like to consider whether the "test statistics" could be offloaded to a plugin. In my opinion it is not quite a "core" feature but would certainly be useful to some users!

If you are open to this, I can help set it up as a plugin, and dedicate some time to looking into what is going on with the query efficencies here...

Hi @SchrodingersGat

Sure I am open to transforming it to plugin. The main intention is to decouple it from the main repo to get rid from it to reduce the maintenance burden, or the plan is the same as the printing plugins: making them optional?

I did not have time to work on InvenTree recently, but I hope this will change in the near future.

@SchrodingersGat SchrodingersGat modified the milestones: 0.17.0, 1.0.0 Nov 12, 2024
@SchrodingersGat SchrodingersGat added the plugin Plugin ecosystem label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Relates to the API backport Apply this label to a PR to enable auto-backport action plugin Plugin ecosystem refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants