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

[WIP] fix isvc for all kind of storage #30

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mwaykole
Copy link
Member

@mwaykole mwaykole commented Oct 30, 2024

is func should work with AWS , GCP , OCI stoarge

@mwaykole mwaykole added the kind/enhancement New feature or request label Oct 30, 2024
@mwaykole mwaykole self-assigned this Oct 30, 2024
storage_uri: str,
model_format: str,
runtime: str,
min_replicas: int = 1,
max_replicas: Optional[int] = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

"""Optional[X] is equivalent to Union[X, None].""" so should be

Suggested change
max_replicas: Optional[int] = 1,
max_replicas: Optional[int] = None,

and in the code, add to the dict if max_replicas is not None

Comment on lines +20 to +23
cpu_limit: str = "2",
memory_limit: str = "8Gi",
cpu_request: str = "1",
memory_request: str = "4Gi",
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we want to set these defaults? why not have the same values set for limits and requests? what happens if you do not set defaults?


if model_service_account:
predictor_config["serviceAccountName"] = model_service_account

Copy link
Contributor

Choose a reason for hiding this comment

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

base_annotations = {
            "serving.kserve.io/deploymentMode": deployment_mode,
        }

if deployment_mode == "RawDeployment":
    annotations = base_annotations
    ....
else:
    annotations = base_annotations.update(....)

}
labels = {
"networking.kserve.io/visibility": "enable-route",
"security.opendatahub.io/enable-auth": "true",
Copy link
Contributor

Choose a reason for hiding this comment

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

should be set based on enable_auth right?

"networking.kserve.io/visibility": "enable-route",
"security.opendatahub.io/enable-auth": "true",
}
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

"""
elif deployment_mode == "Serverless"
"""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants