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

feat(dataflow): Add pipeline version to Kafka headers #5493

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

KengoA
Copy link
Contributor

@KengoA KengoA commented Apr 4, 2024

What this PR does / why we need it:

Currently pipeline versions are not exposed via envoy when you send an inference request. This PR adds a x-pipeline-version to Kafka headers which is then picked up by envoy to return the pipeline information.

Testing steps

Edit seldonruntime CR to specify the image: kengoa/seldon-dataflow-engine:5493 for dataflow engine
Following the steps from https://github.com/SeldonIO/seldon-core/blob/v2/samples/pipeline-versions.md, with an additional argument --show-headers for the infer commands.

After adding a pipeline with the add10 model with seldon pipeline load -f ./pipelines/version-test-a.yaml,

$ seldon pipeline infer version-test --show-headers --inference-mode grpc  '{"model_name":"outlier","inputs":[{"name":"INPUT","contents":{"fp32_contents":[1,2,3,4]},"datatype":"FP32","shape":[4]}]}'

> /inference.GRPCInferenceService/ModelInfer HTTP/2
> Host: 172.18.255.2:80
> seldon-model:[version-test.pipeline]

< x-envoy-upstream-service-time:[120]
< x-pipeline-version:[1]
< date:[Thu, 04 Apr 2024 15:25:05 GMT]
< server:[envoy]
< content-type:[application/grpc]
< x-forwarded-proto:[http]
< x-request-id:[version-test.co7cckgg92tc73eg50tg]
< x-seldon-route:[:add10_1: :version-test.pipeline:]

{"outputs":[{"name":"OUTPUT", "datatype":"FP32", "shape":["4"], "contents":{"fp32Contents":[11, 12, 13, 14]}}]}

it shows x-pipeline-version: [1] as expected.

Applying seldon pipeline load -f ./pipelines/version-test-b.yaml with the mul10 model and running the same infer command produces:

$ seldon pipeline infer version-test  --show-headers --inference-mode grpc  '{"model_name":"outlier","inputs":[{"name":"INPUT","contents":{"fp32_contents":[1,2,3,4]},"datatype":"FP32","shape":[4]}]}'
> /inference.GRPCInferenceService/ModelInfer HTTP/2
> Host: 172.18.255.2:80
> seldon-model:[version-test.pipeline]

< x-seldon-route:[:mul10_1: :version-test.pipeline:]
< x-pipeline-version:[2]
< x-forwarded-proto:[http]
< date:[Thu, 04 Apr 2024 15:25:17 GMT]
< server:[envoy]
< content-type:[application/grpc]
< x-request-id:[version-test.co7ccn8g92tc73eg50u0]
< x-envoy-upstream-service-time:[163]

{"outputs":[{"name":"OUTPUT", "datatype":"FP32", "shape":["4"], "contents":{"fp32Contents":[10, 20, 30, 40]}}]}

which reflects the correct pipeline version with the right model name and output.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@KengoA KengoA added the v2 label Apr 4, 2024
@KengoA KengoA marked this pull request as ready for review April 4, 2024 15:34
@@ -10,7 +10,8 @@ the Change License after the Change Date as each is defined in accordance with t
package io.seldon.dataflow.kafka.headers

object SeldonHeaders {
const val PIPELINENAME = "pipeline"
const val PIPELINE_NAME = "pipeline"
const val PIPELINE_VERSION = "x-pipeline-version"
Copy link
Contributor Author

@KengoA KengoA Apr 4, 2024

Choose a reason for hiding this comment

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

x- prefix is needed here such that convertKafkaHeadersToHttpheaders and convertKafkaheadersToGrpcMetadata pick this up in
scheduler/pkg/kafka/pipeline/utils.go:

if strings.HasPrefix(strings.ToLower(kafkaHeader.Key), resources.ExternalHeaderPrefix) {

External header prefix is appropriate here imo as the pipeline version is coming from an external component (dataflow engine) to envoy.

This PR only intends to display this information and do not do any routing based on pipeline versions, but if we decide to do so in a subsequent PR we can remove this header and have this information as part of x-seldon-route.

Copy link
Member

@sakoush sakoush left a comment

Choose a reason for hiding this comment

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

LGTM. Could you please add / update docs somewhere with this new information so that users know about this behaviour?

@KengoA KengoA merged commit 3ce7029 into SeldonIO:v2 Apr 5, 2024
2 checks passed
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