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

#1553 Add check for stale data in session storage for selected bias charts #2009

Merged

Conversation

alexcreasy
Copy link
Contributor

@alexcreasy alexcreasy commented Oct 24, 2023

Closes:

Description

When the bias page renders, any selected charts in the browser session storage are validated to ensure they still exist i.e. they haven't been deleted by a direct call to a backend API.

How Has This Been Tested?

Pre-requisites:

  • A DSProject with:
    • TrustyAI installed
    • a demo model installed
    • some demo data sent to the model
    • at least one bias metric configured

Instructions are for Chrome, it's possible with any browser but you'll have to figure out manipulating the session storage yourself in any others:

  1. Navigate to the bias metrics tab
  2. From the selector, select a chart
  3. Open the browser console (F12) -> Applications tab
  4. From the 'storage' group on the left menu bar, expand 'Session Storage'
  5. In the main tab, look for a key that will be of the format: 'odh.dashboard.xai.selected_bias_charts-$MODEL_NAME'
  6. Copy the key and value into a text editor for later use.
  7. Back on the dashboard, click the configure button and delete the bias metric you selected in step 2.
  8. Click the "back to $MODEL_NAME" button in the top right to return to the charts section.
  9. Go back to the browser console -> applications -> session storage like in steps 3 & 4
  10. Re-add the key and value you pasted in steps 5 & 6
  11. refresh the page.
  12. Validate that the Metrics to display dropdown is both disabled and does not show any selected charts.

Test Impact

This is a small bug fix and there are no tests as yet for this feature.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

Use image: quay.io/acreasy/odh-dashboard:issue-1553

@openshift-ci openshift-ci bot requested review from dpanshug and uidoyen October 24, 2023 12:18
@alexcreasy
Copy link
Contributor Author

/HOLD

There's a bug in this, apparently I forgot the basics of equality by reference 🤦‍♂️ - this isn't going to work after an object is deserialised from the session store. I'll raise a fix shortly.

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Oct 27, 2023
@alexcreasy alexcreasy force-pushed the issue-1533 branch 2 times, most recently from 49c7e55 to cb2c4a3 Compare October 30, 2023 19:27
…t when metrics are deleted by direct call to the TrustyAI API (opendatahub-io#1533)
@alexcreasy alexcreasy removed the do-not-merge/hold This PR is hold for some reason label Oct 30, 2023
@alexcreasy
Copy link
Contributor Author

This is ready for review now

@pnaik1
Copy link
Contributor

pnaik1 commented Nov 1, 2023

Before:

Screenshot from 2023-11-01 13-15-30

After:

Screenshot from 2023-11-01 12-58-37

Working Fine
@alexcreasy is there anything else I need to check here?

@alexcreasy
Copy link
Contributor Author

@pnaik1 no that works as expected, if you could do a code review and give it an lgtm if you're happy that would be great!

@pnaik1
Copy link
Contributor

pnaik1 commented Nov 1, 2023

code looks good
/lgtm

@alexcreasy
Copy link
Contributor Author

/APPROVE

Copy link
Contributor

openshift-ci bot commented Nov 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexcreasy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Nov 1, 2023
@openshift-ci openshift-ci bot merged commit b37db5e into opendatahub-io:f/mserving-metrics Nov 1, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants