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

fix: allow sort of metrics under avg_metrics [DET-8408] #5086

Merged
merged 3 commits into from
Sep 21, 2022
Merged

fix: allow sort of metrics under avg_metrics [DET-8408] #5086

merged 3 commits into from
Sep 21, 2022

Conversation

mapmeld
Copy link
Contributor

@mapmeld mapmeld commented Sep 21, 2022

Description

Experiment page's Workloads table was having trouble sorting by metric. Discovered this was due to metrics being wrapped inside of metrics->avg_metrics since #4746 . This change attempts to sort by metrics->>key and metrics->avg_metrics->>key. For some reason I also had to cast this to a numeric type to sort by value.

Test Plan

https://determinedai.atlassian.net/browse/DET-8408 describes an experiment which generates metrics and is always sorted server-side by batch number, which then confusingly gets sorted client-side.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.
  • If modifying /webui/react/src/shared/ verify make -C webui/react test-shared passes.

@netlify
Copy link

netlify bot commented Sep 21, 2022

Deploy Preview for storybook-det ready!

Name Link
🔨 Latest commit 12c6b75
🔍 Latest deploy log https://app.netlify.com/sites/storybook-det/deploys/632b630a521eaf000993e59c
😎 Deploy Preview https://deploy-preview-5086--storybook-det.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 settings.

@netlify
Copy link

netlify bot commented Sep 21, 2022

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 12c6b75
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/632b630a521eaf000993e599
😎 Deploy Preview https://deploy-preview-5086--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 settings.

@ioga
Copy link
Contributor

ioga commented Sep 21, 2022

good catch. we need an automated test for this.

This change attempts to sort by metrics->>key and metrics->avg_metrics->>key

why is metrics->>key sorting necessary at all?

@mapmeld
Copy link
Contributor Author

mapmeld commented Sep 21, 2022

why is metrics->>key sorting necessary

Have all past and future recorded metrics been converted to metrics->avg_metrics->>key ?

@ioga
Copy link
Contributor

ioga commented Sep 21, 2022

why is metrics->>key sorting necessary

Have all past and future recorded metrics been converted to metrics->avg_metrics->>key ?

database representation of these hasn't changed, it's all in training/validation CTEs in the queries.
they either use raw metrics column for batch metrics (in which case it has avg_metrics natively), or we just pick the avg_metrics component from it using jsonb_build_object. unless there's a bug in the query, I believe it should all be metrics->avg_metrics.

@mapmeld
Copy link
Contributor Author

mapmeld commented Sep 21, 2022

@ioga - OK that works. Added a client-side fix for the remaining issue

@mapmeld mapmeld enabled auto-merge (squash) September 21, 2022 19:16
@mapmeld mapmeld merged commit 399435c into determined-ai:master Sep 21, 2022
@dannysauer dannysauer modified the milestones: 0.0.102, 0.19.5 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants