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

Construct proper S3 URIs on register_model #39

Merged
merged 3 commits into from
Mar 22, 2024
Merged

Construct proper S3 URIs on register_model #39

merged 3 commits into from
Mar 22, 2024

Conversation

isinyaaa
Copy link
Contributor

@isinyaaa isinyaaa commented Mar 13, 2024 •
edited
Loading

Description

Provides an S3 URI builder util function with overloads that it can guarantee there's no metadata (e.g. from the environment) missing upon creating MR objects, either for UI or deployment purposes.

The usage of the helper is, although, optional. Proposed usage is presented on an example on the README file.

This work is based of off: https://issues.redhat.com/browse/RHOAIENG-3979

How Has This Been Tested?

Inside the client folder, run

poetry run pytest -vv --cov

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or 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

@isinyaaa isinyaaa force-pushed the s3-uri-builder branch 5 times, most recently from ae85d22 to e07aeff Compare March 14, 2024 14:43
@isinyaaa isinyaaa requested review from rareddy and tarilabs March 19, 2024 17:41
tarilabs
tarilabs reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

Thank you for the iterations on this @isinyaaa !

How about a complete example of using this in clients/python/tests/test_client.py ?

to me, already
/lgtm

clients/python/src/model_registry/_client.py Outdated Show resolved Hide resolved
@google-oss-prow google-oss-prow bot added the lgtm label Mar 20, 2024
@google-oss-prow google-oss-prow bot removed the lgtm label Mar 21, 2024
@isinyaaa isinyaaa marked this pull request as ready for review March 21, 2024 15:42
@isinyaaa isinyaaa requested a review from tarilabs March 21, 2024 15:42
@google-oss-prow google-oss-prow bot requested a review from zijianjoy March 21, 2024 15:42
@isinyaaa
Copy link
Contributor Author

isinyaaa commented Mar 21, 2024 •
edited
Loading

How about a complete example of using this in clients/python/tests/test_client.py ?

Sorry, I'll update the tests now, I missed this somehow.

isinyaaa
isinyaaa commented Mar 21, 2024
View reviewed changes
clients/python/src/model_registry/utils.py
Comment on lines +91 to +92
# FIXME: is this safe?
return f"s3://{bucket}/{path}?endpoint={endpoint}&defaultRegion={region}"
Copy link
Contributor Author

@isinyaaa isinyaaa Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

bumping this note here now that the PR is "ready", can you share your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks better for uri building. Did we also want to add the URI components as individual properties to the model metadata? Maybe we can pass a metadata object to the uri builders so that it can be reused in the register call.
@tarilabs didn't UI say they would like the uri components individually, or are they happy to have the constructed uri?

tarilabs
tarilabs reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

Thank you for having expanded on examples and tests!

per #39 (review),
/lgtm

clients/python/tests/test_client.py
Comment on lines +40 to +46
rm = mr_client.register_model(
name,
uri,
model_format_name="test_format",
model_format_version="test_version",
version=version,
)
Copy link
Member

Choose a reason for hiding this comment

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

nit, but superflous; I believe here is missing storage_path, ie:

Suggested change
rm = mr_client.register_model(
name,
uri,
model_format_name="test_format",
model_format_version="test_version",
version=version,
)
rm = mr_client.register_model(
name,
uri,
model_format_name="test_format",
model_format_version="test_version",
storage_path="storage/path",
version=version,
)

@google-oss-prow google-oss-prow bot added the lgtm label Mar 21, 2024
dhirajsb
dhirajsb approved these changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@dhirajsb dhirajsb left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-oss-prow Google OSS Prow
Copy link

@dhirajsb: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tarilabs
tarilabs approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

Thank you @isinyaaa
/approve

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhirajsb, isinyaaa, tarilabs

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

@google-oss-prow google-oss-prow bot merged commit 1ef7b3c into kubeflow:main Mar 22, 2024
10 checks passed
@isinyaaa isinyaaa deleted the s3-uri-builder branch March 22, 2024 17:40
This was referenced Mar 26, 2024
Merged
Merged
rkubis pushed a commit to rkubis/model-registry that referenced this pull request Apr 2, 2024
@tonyxrmdavidson
Adding wait between deployments (kubeflow#39)
57a918b 
* Adding wait between deployments

* added 0 as arg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tarilabs tarilabs tarilabs approved these changes

@dhirajsb dhirajsb dhirajsb approved these changes

@andreyvelich andreyvelich Awaiting requested review from andreyvelich

@Tomcli Tomcli Awaiting requested review from Tomcli

@rareddy rareddy Awaiting requested review from rareddy

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

Assignees

@tarilabs tarilabs

Labels
approved lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@isinyaaa @dhirajsb @rareddy @tarilabs