Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf: remove duplicate ids in ExpMetricNames api #9848

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

keita-determined
Copy link
Contributor

@keita-determined keita-determined commented Aug 20, 2024

Ticket

https://hpe-aiatscale.slack.com/archives/CLCE8D998/p1724182588030219

Description

Remove duplicate ids to improve API performance.
In a certain environment that has relatively lots of run data (50k~), v1/experiments/metrics-stream/metric-names takes some time. For example, https://gcloud.determined.ai/api/v1/experiments/metrics-stream/metric-names?ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=2&ids=3&ids=4&ids=5 takes 6s. It would take longer if there are more run data or more ids query parameters.
This PR is to improve the API response speed performance by removing the duplicate ids. (Sometimes the client (web frontend or user) call the API with duplicate ids)

After this fix, the above API call takes around 1.8s, so 3 times faster than before. We can do some more improvement around the API, but this is the first step.

hey -n 15 -c 1 \
  -H 'Cache-Control: no-cache' \
  -H 'Cookie: [redacted]' \
  -H 'Pragma: no-cache'\
  -H 'Authorization: Bearer token' \ 
'https://gcloud.determined.ai/api/v1/experiments/metrics-stream/metric-names?ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=1&ids=2&ids=3&ids=4&ids=5'

Before

before

After

after

Test Plan

  • call ExpMetricNames api with duplicate ids, and check if duplicated ids are removed in the API function
    • id: [1, 1, 1, 3, 4, 1] -> [1, 3, 4]

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Aug 20, 2024
@keita-determined keita-determined marked this pull request as ready for review August 20, 2024 20:51
@keita-determined keita-determined requested a review from a team as a code owner August 20, 2024 20:51
Copy link

netlify bot commented Aug 20, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit c645aeb
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66c501e30213e7000867005d
😎 Deploy Preview https://deploy-preview-9848--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.71%. Comparing base (e13de20) to head (c645aeb).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9848   +/-   ##
=======================================
  Coverage   54.70%   54.71%           
=======================================
  Files        1261     1261           
  Lines      156012   156014    +2     
  Branches     3588     3588           
=======================================
+ Hits        85353    85357    +4     
+ Misses      70527    70525    -2     
  Partials      132      132           
Flag Coverage Δ
backend 45.16% <100.00%> (+<0.01%) ⬆️
harness 72.60% <ø> (ø)
web 54.47% <ø> (ø)

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

Files Coverage Δ
master/internal/api_experiment.go 56.80% <100.00%> (+0.04%) ⬆️

... and 4 files with indirect coverage changes

@keita-determined keita-determined added the to-cherry-pick Pull requests that need to be cherry-picked into the current release label Aug 20, 2024
Copy link
Contributor

@maxrussell maxrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@keita-determined keita-determined merged commit 32fafdd into main Aug 20, 2024
91 of 104 checks passed
@keita-determined keita-determined deleted the perf/remove-dupe branch August 20, 2024 21:26
github-actions bot pushed a commit that referenced this pull request Aug 20, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cla-signed to-cherry-pick Pull requests that need to be cherry-picked into the current release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants