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

Keep up with midstream #706

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Conversation

lucferbux
Copy link
Contributor

@lucferbux lucferbux commented Jan 16, 2025

Description

This PR intends to update the functionality of MR by updating the codebase with latest Midstream changes.

There are some UI inconsistencies that we'll need to tweak cc @jenny-s51

Please take a look at all the pages in the review.

Screenshots Screenshot 2025-01-17 at 14 14 56 Screenshot 2025-01-17 at 14 15 08 Screenshot 2025-01-17 at 14 14 48 Screenshot 2025-01-17 at 14 15 19 Screenshot 2025-01-17 at 14 15 28 Screenshot 2025-01-17 at 14 15 38 Screenshot 2025-01-17 at 14 15 56

How Has This Been Tested?

Updated all testing, both unit and integration. Tested manually too.

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

@lucferbux lucferbux force-pushed the rhoaieng-15608 branch 6 times, most recently from eedb41a to 67c3c8c Compare January 17, 2025 13:13
@lucferbux lucferbux marked this pull request as ready for review January 17, 2025 13:13
@google-oss-prow google-oss-prow bot requested a review from alexcreasy January 17, 2025 13:14
@lucferbux lucferbux force-pushed the rhoaieng-15608 branch 3 times, most recently from f450362 to f490ecd Compare January 17, 2025 14:12
@lucferbux lucferbux force-pushed the rhoaieng-15608 branch 4 times, most recently from 3a19a27 to ebd051e Compare January 20, 2025 16:54
@Griffin-Sullivan
Copy link
Contributor

One bug is if you are on the main page and select the drop down next to Register Model:
Screenshot from 2025-01-20 12-09-00

Clicking Register new version will take you to:
Screenshot from 2025-01-20 12-09-22

Seems like the call to register a new version will have no registeredModelId so it doesn't know where to register the version to.

@lucferbux
Copy link
Contributor Author

lucferbux commented Jan 21, 2025

One bug is if you are on the main page and select the drop down next to Register Model: Screenshot from 2025-01-20 12-09-00

Clicking Register new version will take you to: Screenshot from 2025-01-20 12-09-22

Seems like the call to register a new version will have no registeredModelId so it doesn't know where to register the version to.

After checking the code, it seems that this is a bug in midstream, i need to check it there but it seems that's the case.

[EDIT] Found the issue, and it was my fault, should silently fail the NotReadyError, I changed that in the first refactor, should stick to the logic in midstream, already fixed.

Copy link
Contributor

@alexcreasy alexcreasy left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the code but tested locally in kind and it works great!

Nice job!

Copy link

[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

@alexcreasy
Copy link
Contributor

As Griffen did a thorough code review and we've both tested it. merging.

/gtm

@alexcreasy
Copy link
Contributor

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 22, 2025
@google-oss-prow google-oss-prow bot merged commit 979b1ed into kubeflow:main Jan 22, 2025
16 checks passed
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