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

perf: improve upsert responses bulk #4451

Merged

Conversation

frascuchon
Copy link
Member

@frascuchon frascuchon commented Dec 22, 2023

Description

This PR applies some performance improvements related to the bulk upsert responses endpoint since some environments are experimenting with a high execution time when running this operation with 100 items in the request.

The performance improvements are:

  1. Read all requested records at once instead of once at a time.
  2. Stop checking the existence of the search index since is taking longer.

These improvements are applied based on the profiling results using the test ...

Also, since the current API controller is doing many things, a new design proposal is applied in the implementation by defining a simple use case implementation for this functionality. The use case wraps all the needed steps to cover the functionality and make the controller implementation quite simple, focusing only on exception handling and request/response serialization.

Closes #4449

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

  • New feature (non-breaking change which adds functionality)
  • Refactor (change restructuring the codebase without changing functionality)
  • Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

Test locally

Checklist

  • I added relevant documentation
  • I followed the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints language: python Pull requests or issues that update Python code team: backend Indicates that the issue or pull request is owned by the backend team type: improvement Indicates updates or improvements on existing features labels Dec 22, 2023
@frascuchon frascuchon changed the title Perf/improve upsert responses bulk perf: improve upsert responses bulk Dec 22, 2023
@frascuchon frascuchon force-pushed the perf/improve-upsert-responses-bulk branch from d81909a to be6612a Compare December 22, 2023 17:25
@frascuchon frascuchon self-assigned this Dec 22, 2023
@frascuchon frascuchon added this to the v1.22.0 milestone Dec 22, 2023
Copy link

The URL of the deployed environment for this PR is https://argilla-quickstart-pr-4451-ki24f765kq-no.a.run.app

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e3f0992) 66.16% compared to head (974cd30) 91.36%.
Report is 136 commits behind head on feature/bulk-annotation.

Files Patch % Lines
src/argilla/server/apis/v1/handlers/responses.py 77.77% 2 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           feature/bulk-annotation    #4451       +/-   ##
============================================================
+ Coverage                    66.16%   91.36%   +25.19%     
============================================================
  Files                          335      340        +5     
  Lines                        19333    19822      +489     
============================================================
+ Hits                         12792    18110     +5318     
+ Misses                        6541     1712     -4829     

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

@damianpumar
Copy link
Contributor

Great work @frascuchon discarding 100 records.
Screenshot 2023-12-22 at 23 39 41

@frascuchon frascuchon requested a review from gabrielmbmb January 8, 2024 10:51
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 8, 2024
@frascuchon frascuchon merged commit 59fe822 into feature/bulk-annotation Jan 8, 2024
20 checks passed
@frascuchon frascuchon deleted the perf/improve-upsert-responses-bulk branch January 8, 2024 11:08
@frascuchon frascuchon linked an issue Jan 9, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints language: python Pull requests or issues that update Python code lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files. team: backend Indicates that the issue or pull request is owned by the backend team type: improvement Indicates updates or improvements on existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Improve response upsert bulk endpoint performance
3 participants