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.

Sign up for GitHub

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

Jump to bottom

feat: add basic lineage MDLM link #9482

Merged
merged 21 commits into from
Jul 12, 2024
Merged

feat: add basic lineage MDLM link #9482

merged 21 commits into from
Jul 12, 2024

Conversation

thiagodallacqua-hpe
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe commented Jun 6, 2024 •
edited
Loading

Ticket: ET-252

Description

Adds the MDLM link to experiments that has integrations data linked to it.

Here's the expected placements:

  • in the single run view when the run is based off of an MLDM model

  • in the checkpoint modal view when the checkpoint’s experiment is based off of an MLDM model

  • in the model view when the model’s checkpoint’s experiment is based off of an MLDM model

Test Plan

the easiest way to test this is to fork the experiment core-api-stage-0 and add the following to its config:

integrations:
   pachyderm:
      proxy:
         scheme: http
         host: localhost 
         port: 80
      pachd:
         host: localhost
         port: 30650
      dataset:
         repo: test-data
         project: test-project
         branch: master
         commit: 1d2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b
         token: 1234567890abcdef1234567890abcdef

Screenshot 2024-06-20 at 14 12 34

Screenshot 2024-06-20 at 14 12 17

Screenshot 2024-06-20 at 13 23 12

From there it should be possible to add the checkpoint and model based on the forked experiment and have the MDLM link in the experiment details page, checkpoint modal and model view.

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 Jun 6, 2024
@netlify Netlify
Copy link

netlify bot commented Jun 6, 2024 •
edited
Loading

✅ Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit c9a9588
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66916d474f8a5d0008bf9059
😎 Deploy Preview https://deploy-preview-9482--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.

@thiagodallacqua-hpe thiagodallacqua-hpe marked this pull request as ready for review June 18, 2024 14:28
@thiagodallacqua-hpe thiagodallacqua-hpe requested a review from a team as a code owner June 18, 2024 14:28
@thiagodallacqua-hpe thiagodallacqua-hpe requested review from ashtonG and removed request for EmilyBonar June 20, 2024 16:26
@thiagodallacqua-hpe thiagodallacqua-hpe force-pushed the thiago/ET-252 branch 2 times, most recently from c28259b to 3a44ae1 Compare June 20, 2024 18:54
ashtonG
ashtonG requested changes Jun 24, 2024
View reviewed changes
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.

i don't think the config validation requires a pachyderm key if it includes integrations (that is, an empty object or an object with keys other than pachyderm is a valid integrations setup). this should take that into account.

webui/react/src/types.ts Outdated Show resolved Hide resolved
webui/react/src/types.ts Outdated Show resolved Hide resolved
@thiagodallacqua-hpe
attend to PR comments:
c162424 
Change base type for the integration and made necessary followup changes
ashtonG
ashtonG approved these changes Jul 2, 2024
View reviewed changes
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.

looks fine, just had one more nit

keita-determined
keita-determined previously requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@keita-determined keita-determined left a comment •
edited
Loading

Choose a reason for hiding this comment

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

i just noticed few things, so added comments. i think its a blocker
also some test file covs are under 80% or do we care about overall cov? iirc, file cov?

webui/react/src/pages/TrialDetails/TrialInfoBox.test.tsx Outdated
Comment on lines 242 to 252
waitFor(() => {
expect(screen.findByText('Data Input')).toBeVisible();
expect(screen.findByText(mockIntegrationData.dataset.repo)).toBeVisible();
});
});

it('should not show Data input card when pachyderm integration is missing', () => {
setup(mockTrial1, mockExperiment);
waitFor(() => {
expect(screen.findByText('Data Input')).not.toBeVisible();
});
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 these test cases actually dont assert values. even if text is changed to whatever values, the test cases pass. feels like this needs to be fixed before merging this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In here I'm validating it by using the findByText with the exact same mock data set to be the repo's name... the test for the URL that's being built based on the lineage values is being done in the integrations.test.ts, which asserts for the exact URL as it is expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

the test cases are false negative. can we fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to use data-testid in order to abstract the text values from it

Copy link
Contributor

Choose a reason for hiding this comment

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

changed to use data-testid in order to abstract the text values from it

i cant find test cases that use data-testid. i think its missing

keita-determined
keita-determined reviewed Jul 12, 2024
View reviewed changes
webui/react/src/pages/TrialDetails/TrialInfoBox.test.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/TrialDetails/TrialInfoBox.test.tsx Outdated
Comment on lines 242 to 252
waitFor(() => {
expect(screen.findByText('Data Input')).toBeVisible();
expect(screen.findByText(mockIntegrationData.dataset.repo)).toBeVisible();
});
});

it('should not show Data input card when pachyderm integration is missing', () => {
setup(mockTrial1, mockExperiment);
waitFor(() => {
expect(screen.findByText('Data Input')).not.toBeVisible();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

changed to use data-testid in order to abstract the text values from it

i cant find test cases that use data-testid. i think its missing

@thiagodallacqua-hpe thiagodallacqua-hpe merged commit 6299dcd into main Jul 12, 2024
81 of 94 checks passed
@thiagodallacqua-hpe thiagodallacqua-hpe deleted the thiago/ET-252 branch July 12, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keita-determined keita-determined keita-determined left review comments

@ashtonG ashtonG ashtonG approved these changes

Assignees

@ashtonG ashtonG

Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thiagodallacqua-hpe @ashtonG @keita-determined @EmilyBonar