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

feat: support filter in flat run table #9250

Merged
merged 5 commits into from
May 6, 2024

Conversation

keita-determined
Copy link
Contributor

@keita-determined keita-determined commented Apr 26, 2024

Ticket

ET-111

Description

Add filter feature in Flat Run table (Run table is under feature flag)

Test Plan

  • Enable feature flag of Flat Runs View
  • Go to flat run table
  • Check if
    • filter button is visible and usable on top of the run table
    • filter is saved after applying valid filters, so filtered table is preserved even after refreshing the page
    • filter should be able to be applied from table header
    • filter should be able to be cleared from table header
      (filter behavior should be the same as the experiment table we have now (its not feature flagged))

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.

@cla-bot cla-bot bot added the cla-signed label Apr 26, 2024
Copy link

netlify bot commented Apr 26, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 0630fad
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66341ebd69f67a0008a5c684
😎 Deploy Preview https://deploy-preview-9250--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 Apr 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 151 lines in your changes are missing coverage. Please review.

Project coverage is 37.91%. Comparing base (e31135a) to head (0630fad).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9250      +/-   ##
==========================================
- Coverage   44.60%   37.91%   -6.69%     
==========================================
  Files        1275      951     -324     
  Lines      156240   116539   -39701     
  Branches     2450     2450              
==========================================
- Hits        69687    44183   -25504     
+ Misses      86313    72116   -14197     
  Partials      240      240              
Flag Coverage Δ
harness ?
web 34.99% <0.00%> (-0.06%) ⬇️

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

Files Coverage Δ
webui/react/src/components/TableActionBar.tsx 0.00% <0.00%> (ø)
...bui/react/src/pages/F_ExpList/F_ExperimentList.tsx 0.00% <0.00%> (ø)
...ui/react/src/components/FilterForm/TableFilter.tsx 0.00% <0.00%> (ø)
...ebui/react/src/pages/FlatRuns/FlatRuns.settings.ts 0.00% <0.00%> (ø)
webui/react/src/pages/FlatRuns/FlatRuns.tsx 0.00% <0.00%> (ø)

... and 324 files with indirect coverage changes

@keita-determined keita-determined marked this pull request as ready for review April 30, 2024 21:38
@keita-determined keita-determined requested a review from a team as a code owner April 30, 2024 21:38
@keita-determined keita-determined changed the title feat: support filter in run table feat: support filter in flat run table Apr 30, 2024
Copy link
Contributor

@johnkim-det johnkim-det left a comment

Choose a reason for hiding this comment

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

  • noticed that Searcher Metrics Value does not show filter options in header menu but it does still appear in the Filters dropdown menu. This is true for Experiments List, too. But maybe worth addressing here?

  • Also, it looks like the Searcher ID and Searcher Description columns appear in the table but can't be filtered, even though they're not in BANNED_FILTER_COLUMNS (and I don't think we would want them to be?). I think we just need to make sure everything in getColumnDefs and runColumns matches. May need to update again with feat: Add Run columns to GetProjectColumns #9146 but should probably make sure current columns work with filters as expected.

  • Also not sure why, but I don't think ID filter is working. Returns no results even when I'm expecting one to be returned.

webui/react/src/pages/FlatRuns/FlatRuns.tsx Outdated Show resolved Hide resolved
@keita-determined
Copy link
Contributor Author

keita-determined commented May 2, 2024

  • noticed that Searcher Metrics Value does not show filter options in header menu but it does still appear in the Filters dropdown menu. This is true for Experiments List, too. But maybe worth addressing here?

fixed in run table and experiment table

  • Also, it looks like the Searcher ID and Searcher Description columns appear in the table but can't be filtered, even though they're not in BANNED_FILTER_COLUMNS (and I don't think we would want them to be?). I think we just need to make sure everything in getColumnDefs and runColumns matches. May need to update again with feat: Add Run columns to GetProjectColumns #9146 but should probably make sure current columns work with filters as expected.

Searcher ID and Searcher Description filters exit.
in

, but the column names are overwritten, so Searcher ID filter exits as ID. Search Name filter exits as Name. Searcher Description filter exits as Description.
imo these names shouldn't be overwritten in frontend. that causes the name mismatching like this.

@johnkim-det what do you think is the best solution for this? If we wanna keep the current column names, we need to fix backend. if we wanna change the column name, these should match the column data from backend. i think backend change is required for either way

  • Also not sure why, but I don't think ID filter is working. Returns no results even when I'm expecting one to be returned.

ID means run id instead of experiment id? looks like backend doesn't support it yet. I don't think frontend can do much with it afaik.

@johnkim-det
Copy link
Contributor

yeah I think it makes sense to always use the names from the backend -- might be better to read the names from projectColumns in getColumnDefs instead of using title field.

But I'm less concerned with the mismatch of column names between filters menu and column headers than the fact that the column header menus aren't displaying filter options at all for those columns. the name mismatch shouldn't cause that, right?

@keita-determined
Copy link
Contributor Author

But I'm less concerned with the mismatch of column names between filters menu and column headers than the fact that the column header menus aren't displaying filter options at all for those columns. the name mismatch shouldn't cause that, right?

Right, the name mismatch doesnt cause the issue. Backend doesnt return these columns. thats the issue. i dont think web cant do anything with this until backend return missing columns

Copy link
Contributor

@johnkim-det johnkim-det left a comment

Choose a reason for hiding this comment

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

OK I see now that the filter options are based on projectColumns while displaying the column is based on settings.columns/FE definition. Not sure if it's good to have two different places where columns are defined like that, but I guess it's out of scope of this PR either way.

It does look like #9146 will provide experimentId and experimentDescription in projectColumns. might be a good idea to try those changes with yours to make sure that at least everything filter-related will work.

but will approve this assuming those two columns are the only issues, since they'd be addressed with #9146 merge.

ID filter not working is a different problem, do you mind making a ticket for that so it's captured for grooming discussion?

@keita-determined
Copy link
Contributor Author

made a ticket

@keita-determined keita-determined merged commit fdaa015 into main May 6, 2024
83 of 96 checks passed
@keita-determined keita-determined deleted the feature/run-table-filter branch May 6, 2024 20:58
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.

2 participants