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: add metadata column type badge #10052

Closed
wants to merge 1 commit into from
Closed

Conversation

thiagodallacqua-hpe
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe commented Oct 14, 2024

Ticket

ET-791

Description

Right now, if we have multiple metadata columns with the same name, there's no quick way of knowing of which type each one is while selecting it. This PR intends to add a descriptive badge next to duplicate entries for the metadata columns.

Test Plan

  • make sure that you have at least one metadata column that is duplicated and of a different type
  • go to a experiment list
    • make sure that the grid visualization is enabled
  • open the column picker
  • verify that the duplicates has a <column_name> <column_type_tag> format
Screenshot 2024-10-14 at 16 58 13 Screenshot 2024-10-14 at 16 59 27

Screenshot 2024-10-21 at 11 12 49

Screenshot 2024-10-21 at 11 12 42

Screenshot 2024-10-21 at 11 13 43

Screenshot 2024-10-18 at 17 44 26

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

netlify bot commented Oct 14, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit cbf0bb1
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/672a42e0a84e2600085acf34
😎 Deploy Preview https://deploy-preview-10052--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 Oct 14, 2024

Codecov Report

Attention: Patch coverage is 68.47826% with 29 lines in your changes missing coverage. Please review.

Project coverage is 54.07%. Comparing base (225848a) to head (c801b0c).

Current head c801b0c differs from pull request most recent head cbf0bb1

Please upload reports for the commit cbf0bb1 to get more accurate results.

Files with missing lines Patch % Lines
webui/react/src/pages/FlatRuns/FlatRuns.tsx 3.84% 25 Missing ⚠️
webui/react/src/components/ColumnPickerMenu.tsx 92.15% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10052      +/-   ##
==========================================
- Coverage   54.35%   54.07%   -0.28%     
==========================================
  Files        1259      448     -811     
  Lines      157337    77346   -79991     
  Branches     3644     3654      +10     
==========================================
- Hits        85517    41827   -43690     
+ Misses      71687    35385   -36302     
- Partials      133      134       +1     
Flag Coverage Δ
harness ?
web 54.33% <68.47%> (+0.02%) ⬆️

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

Files with missing lines Coverage Δ
webui/react/src/components/Searches/columns.ts 51.38% <ø> (-0.51%) ⬇️
webui/react/src/pages/F_ExpList/expListColumns.ts 21.23% <ø> (+0.29%) ⬆️
webui/react/src/pages/FlatRuns/columns.ts 18.69% <ø> (+0.20%) ⬆️
webui/react/src/utils/flatRun.ts 99.24% <100.00%> (+0.08%) ⬆️
webui/react/src/components/ColumnPickerMenu.tsx 96.01% <92.15%> (+0.08%) ⬆️
webui/react/src/pages/FlatRuns/FlatRuns.tsx 11.27% <3.84%> (+0.05%) ⬆️

... and 815 files with indirect coverage changes

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.

we wanna show types for all columns.
and keys need to be adjusted.

@keita-determined
Copy link
Contributor

keita-determined commented Oct 18, 2024

can we prioritize this PR over the PR about filter #10046?

webui/react/src/components/ColumnPickerMenu.tsx Outdated Show resolved Hide resolved
webui/react/src/components/ColumnPickerMenu.tsx Outdated Show resolved Hide resolved
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.

Could you make sure this PR displays the columns' data correctly on #10066?
so far, it doesnt show data correctly.

webui/react/src/components/ColumnPickerMenu.tsx Outdated Show resolved Hide resolved
@@ -63,6 +66,12 @@ interface ColumnTabProps {
onHeatmapSelectionRemove?: (id: string) => void;
}

export const formatColumnKey = (col: ProjectColumn): string => {
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 wanna store all columns with types based on the discussion 2 weeks ago.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is basically used for mapping and encoding the UI part, which seems only necessary for the arbitrary metadata columns, as discussed with @ashtonG previously 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

just confirming -- as the only affected columns are run metadata columns, we should be okay to limit the type encoding to run metadata columns.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as i know, hyperparameter would have the same names with different types too. i think its safer to encode all columns for the future and consistency.

@thiagodallacqua-hpe thiagodallacqua-hpe force-pushed the thiago/ET-791 branch 2 times, most recently from 1a7b2bf to 1d1ddd1 Compare October 23, 2024 14:00
@thiagodallacqua-hpe thiagodallacqua-hpe requested review from a team as code owners October 23, 2024 16:51
Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

found a bug -- with multiple types of the same metadata column selected, changing the column width of one column changes the width of all columns with the same name. please make sure the column ids that we're sending to the datagrid component are correct.

@thiagodallacqua-hpe thiagodallacqua-hpe requested review from ashtonG and removed request for jgongd October 23, 2024 17:21
@thiagodallacqua-hpe thiagodallacqua-hpe force-pushed the thiago/ET-791 branch 4 times, most recently from 78dda83 to 5ddcc36 Compare October 23, 2024 19:29
@thiagodallacqua-hpe thiagodallacqua-hpe force-pushed the thiago/ET-791 branch 2 times, most recently from 8eba48a to f6c896e Compare October 24, 2024 14:49
Comment on lines 187 to 180
KNOWN_BOOLEAN_COLUMNS.includes(col.column) && col.type === V1ColumnType.UNSPECIFIED
? 'BOOLEAN'
: removeColumnTypePrefix(col.type);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it turns out that col.column in runColumns returns false even if the column is actually part of it 😅

but, this works

Screenshot 2024-10-24 at 12 45 34

webui/react/src/components/ColumnPickerMenu.tsx Outdated Show resolved Hide resolved
webui/react/src/components/ColumnPickerMenu.tsx Outdated Show resolved Hide resolved
webui/react/src/components/ColumnPickerMenu.tsx Outdated Show resolved Hide resolved
webui/react/src/components/Searches/Searches.tsx Outdated Show resolved Hide resolved
webui/react/src/utils/flatRun.ts Outdated Show resolved Hide resolved
@thiagodallacqua-hpe thiagodallacqua-hpe force-pushed the thiago/ET-791 branch 4 times, most recently from f4aa269 to c801b0c Compare October 31, 2024 14:38
@thiagodallacqua-hpe thiagodallacqua-hpe force-pushed the thiago/ET-791 branch 3 times, most recently from 0a4e9c2 to 9ddcc0e Compare November 1, 2024 17:18
@thiagodallacqua-hpe
Copy link
Contributor Author

Closing PR due to company restructuring.

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