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

fix(agent): Use type from storage config and enforce name matches config #4780

Merged
merged 7 commits into from
Apr 5, 2023

Conversation

agrski
Copy link
Contributor

@agrski agrski commented Apr 4, 2023

Why

Motivation

There are two ways of providing storage configs for models to the Core v2 agent:

  • Directly via the model spec's .spec.storageConfig field, which creates an ad-hoc config for Rclone.
  • Via the agent's config file (or configmap), which then automatically loads all such configs into Rclone.

These mechanisms currently behave somewhat differently.

When use pre-configured configs (not specific to a given model), the assumption is that the model uses a storageUri with a scheme matching a known config. For example, Rclone might have config for a remote called gs and the model might have a storage URI like gs://seldon-models/mlserver/iris.

When using ad-hoc configs, the agent currently assumes the URI scheme (gs in this example) is the Rclone type of the remote. However, we require storage configs to already have a type and, in fact, a name.

This PR brings the ad-hoc behaviour in line with that of the pre-configured configs. The type from the config is used as the Rclone type and the name is expected to match the URI scheme, as Rclone would normally expect. This is simpler, consistent, and avoids confusion about what to call a remote depending on how a config is used.

Issues

N/A

What

Changes

  • Add check that URI scheme matches remote name from storage config for ad-hoc configs.
  • Use config Type as Rclone type for ad-hoc configs.

Testing

I have a simple Core v2 setup with MinIO as an S3 provider. I have set up a storage secret in line with this.
The basic model looks like this:

apiVersion: mlops.seldon.io/v1alpha1
kind: Model
metadata:
  namespace: seldon
  name: iris
spec:
  storageUri: minio://models/iris
  secretName: minio-scv2
  memory: 100Ki
  requirements: ['sklearn', 'python']
name: minio
type: s3
parameters:
  provider: minio
  access_key_id: XXX
  secret_access_key: XXX
  env_auth: false
  endpoint: http://minio.minio-system.svc.cluster.local:9000
  • Valid config secret but non-matching URI fails.
    • "name from URI (s3) does not match secret (minio); are you using the right storage config?"
  • Valid config secret and matching URI works.
    • seldon model infer iris '{"id": "test", "inputs": [{"name": "predict", "shape": [1, 4], "datatype": "FP32", "data": [[4, 0.1, 2.4, 3.3]]}]}' --inference-host XXX

@agrski agrski requested a review from ukclivecox April 4, 2023 14:22
@agrski agrski marked this pull request as ready for review April 4, 2023 16:23
@agrski agrski marked this pull request as draft April 4, 2023 16:28
@agrski agrski force-pushed the always-use-storage-type-from-secrets branch from 644db52 to de7153d Compare April 4, 2023 17:01
@agrski agrski marked this pull request as ready for review April 4, 2023 17:34
@ukclivecox ukclivecox added the v2 label Apr 5, 2023
@agrski agrski force-pushed the always-use-storage-type-from-secrets branch from de7153d to 0fe4ceb Compare April 5, 2023 18:44
@agrski agrski changed the title fix(agent): Use type from storage config fix(agent): Use type from storage config and enforce name matches config Apr 5, 2023
@agrski agrski merged commit ca42423 into SeldonIO:v2 Apr 5, 2023
@agrski agrski deleted the always-use-storage-type-from-secrets branch April 5, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants