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: use post for searchRuns/use accurate multitrial detection #9546

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

ashtonG
Copy link
Contributor

@ashtonG ashtonG commented Jun 20, 2024

Description

this fixes a few issues with the multitrial check for run actions

  • when the selection grew too large, the resulting url triggered a 431 error. we're switching the method of the search runs endpoint to post in order to get around this.
  • the multitrial check used the wrong method to check if a run was part of a search
  • the warning shown if the multitrial check returned positive had incorrect copy

Test Plan

  • navigate to the runs table
  • select all runs in the current page
  • select the move action
  • the check should succeed and the notice should show that multitrial runs will be moved along with other runs in their search

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

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 51.27%. Comparing base (f61cde3) to head (9dbc662).

Additional details and impacted files
@@                       Coverage Diff                       @@
##           feature/actions-flat-run-table    #9546   +/-   ##
===============================================================
  Coverage                           51.27%   51.27%           
===============================================================
  Files                                1252     1252           
  Lines                              152000   151993    -7     
  Branches                             3017     3017           
===============================================================
- Hits                                77937    77934    -3     
+ Misses                              73904    73900    -4     
  Partials                              159      159           
Flag Coverage Δ
backend 43.89% <ø> (-0.01%) ⬇️
harness 72.80% <ø> (ø)
web 47.85% <85.71%> (+<0.01%) ⬆️

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

Files Coverage Δ
...components/RunFilterInterstitialModalComponent.tsx 98.21% <100.00%> (ø)
...ct/src/components/RunMoveWarningModalComponent.tsx 98.18% <100.00%> (+0.03%) ⬆️
webui/react/src/services/apiConfig.ts 73.80% <60.00%> (+0.23%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Contributor

@AmanuelAaron AmanuelAaron left a comment

Choose a reason for hiding this comment

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

Backend LGTM

`%22%2C%22value%22%3A0.1%7D%5D%2C%22conjunction%22%3A%22and%22%2C%22kind` +
`%22%3A%22group%22%7D%2C%22showArchived%22%3Afalse%7D`,
),
testRequest('POST', '/api/v1/runs', JSON.stringify({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should run the perf test once to confirm that this is working. I think it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran it -- needed to stringify the filter param. rerunning now.

@keita-determined
Copy link
Contributor

can we fix the conflicts? i'll review it afterwards

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

i got this error on this branch. i deleted all DB and tested in on the incognito mode
Do i miss something?

image

performance/src/utils/helpers.ts Show resolved Hide resolved
@@ -126,7 +126,7 @@ export const RunFilterInterstitialModalComponent = forwardRef<ControlledModalRef
const results = await searchRuns(
{
filter: JSON.stringify(filter),
limit: 0,
limit: -2,
Copy link
Contributor

Choose a reason for hiding this comment

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

qq: what does -2 mean in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-2 returns empty results, leaving just the pagination.

@ashtonG
Copy link
Contributor Author

ashtonG commented Jun 25, 2024

@keita-determined fixed -- i did a little code golfing before committing which didn't work out.

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

working good with many experiments


out of scope: do we want it in the experiment table as well, or just flat run since we eventually remove the experiment table?

@ashtonG
Copy link
Contributor Author

ashtonG commented Jun 26, 2024

we do, but I want to bundle that work with leveraging some other things we can do to reduce the backend complexity now that the parameters don't have to be strings. here's the ticket: https://hpe-aiatscale.atlassian.net/browse/ET-602

@ashtonG ashtonG merged commit eb25bae into feature/actions-flat-run-table Jun 26, 2024
79 of 94 checks passed
@ashtonG ashtonG deleted the fix/run-actions-fixes branch June 26, 2024 18:03
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.

None yet

3 participants