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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions go/internal/feast/featurestore.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ package feast
import (
"context"
"errors"
"fmt"
"os"
"strings"

"github.com/apache/arrow/go/v8/arrow/memory"

Expand Down Expand Up @@ -58,10 +55,14 @@ func NewFeatureStore(config *registry.RepoConfig, callback transformation.Transf
if err != nil {
return nil, err
}
sanitizedProjectName := strings.Replace(config.Project, "_", "-", -1)
productName := os.Getenv("PRODUCT")
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.

if config.GoTransformationsServer {
if config.GoTransformationsEndpoint == "" {
return nil, errors.New("Transformations server endpoint is missing. Update featue_store.yaml with go_transformations_endpint configuration")
}
transformationService, _ = transformation.NewGrpcTransformationService(config, config.GoTransformationsEndpoint)
}

return &FeatureStore{
config: config,
Expand Down
5 changes: 5 additions & 0 deletions go/internal/feast/registry/repoconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ type RepoConfig struct {
RepoPath string `json:"repo_path"`
// EntityKeySerializationVersion
EntityKeySerializationVersion int64 `json:"entity_key_serialization_version"`
// If false, use gopy bindings to calculate ODFV transformations.
// "True" value required for Go feature server to serve ODFVs with stability and at scale.
GoTransformationsServer bool `json:"go_transformations_server"`
// Transformation server base endpoint.
GoTransformationsEndpoint string `json:"go_transformations_endpoint"`
}

type RegistryConfig struct {
Expand Down
27 changes: 27 additions & 0 deletions go/internal/feast/registry/repoconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,30 @@ online_store:
assert.Equal(t, dir, config.RepoPath)
assert.Equal(t, "data/registry.db", config.GetRegistryConfig().Path)
}

func TestGoTransformationsEndpoint(t *testing.T) {
dir, err := os.MkdirTemp("", "feature_repo_*")
assert.Nil(t, err)
defer func() {
assert.Nil(t, os.RemoveAll(dir))
}()
filePath := filepath.Join(dir, "feature_store.yaml")
data := []byte(`
registry:
path: data/registry.db
project: feature_repo
provider: local
online_store:
type: redis
connection_string: "localhost:6379"
go_transformations_endpoint: https://go.dev:9999
go_transformations_server: True
`)
err = os.WriteFile(filePath, data, 0666)
assert.Nil(t, err)
config, err := NewRepoConfigFromFile(dir)
assert.Nil(t, err)
assert.Equal(t, dir, config.RepoPath)
assert.Equal(t, "https://go.dev:9999", config.GoTransformationsEndpoint)
assert.Equal(t, true, config.GoTransformationsServer)
}
8 changes: 8 additions & 0 deletions sdk/python/feast/repo_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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?

""" If True, use the transformations server to perform ODVF transformations in Go feature server. """

go_transformations_endpoint: Optional[StrictStr] = ""
""" Specify the endpoint for Go feature server to find the transformations server.
NOTE: Unless go_transformations_server is False, the Go feature server will throw errors if this is
blank or null. """

entity_key_serialization_version: StrictInt = 1
""" Entity key serialization version: This version is used to control what serialization scheme is
used when writing data to the online store.
Expand Down
Loading