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: Add transformations server endpoint to feature_store.yaml #85

Closed
wants to merge 10 commits into from

Conversation

wparsley-eg
Copy link

@wparsley-eg wparsley-eg commented Feb 20, 2024

What this PR does / why we need it:
Currently, the transformations server endpoint is only configurable through environment variables. This will allow users to fully specify the endpoint to be used.

Why put this in feature_store.yaml?
I studied the Java feature server, which includes its own transformations server. The Java feature server is a Springboot application, and therefore uses its Helm charts to specify the endpoint and port for the transformations server.
However, there is no Springboot equivalent in Go. Go developers tend to prefer a more spartan style of development and ingesting variables from the Helm charts would have to be done manually, and may not be portable depending on use cases.
However, the Go process already ingests the feature_store.yaml file, and we can easily set the value there. This requires no extra code and should be more portable for open source users.

@wparsley-eg wparsley-eg marked this pull request as ready for review February 20, 2024 00:21
go/internal/feast/registry/repoconfig.go Outdated Show resolved Hide resolved
go/internal/feast/featurestore.go Outdated Show resolved Hide resolved
go/internal/feast/featurestore.go Outdated Show resolved Hide resolved
go/internal/feast/registry/repoconfig.go Outdated Show resolved Hide resolved
@@ -177,6 +177,14 @@ class RepoConfig(FeastBaseModel):
go_feature_retrieval: Optional[bool] = False
""" If True, use the embedded Go code to retrieve features instead of the Python SDK. """

go_transformations_server: Optional[bool] = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Had a look at the repo_config. Noticed feature_server section in repo_config. We should have grouped these configs there instead of separate configs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are adding expedia provider. With that we can always force the flag to TRUE irrespective of what is set in feature_store.yaml. So we can ignore the above comment "Earlier logic was handling: Flag=False and Endpoint=Value, Its using Transformation service. Now it's using GOPY bindings"

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about moving configurations to feature_server section?

endpoint := fmt.Sprintf("%s-transformations.%s.svc.cluster.local:80", sanitizedProjectName, productName)
transformationService, _ := transformation.NewGrpcTransformationService(config, endpoint)

var transformationService *transformation.GrpcTransformationService = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Earlier logic was handling: Flag=False and Endpoint=Value, Its using Transformation service. Now it's using GOPY bindings. Was that the intention when the code was written previously?

Copy link
Author

Choose a reason for hiding this comment

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

No, this version of the code is what should be done. If flag=false, we should be using GOPY bindings. Flag should control whether we use the transformations server or not.

@wparsley-eg
Copy link
Author

Closing in favor of this pr

@wparsley-eg wparsley-eg closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants