chore: Use Go-based implementation as embedded extension to Python SDK#2429
chore: Use Go-based implementation as embedded extension to Python SDK#2429feast-ci-bot merged 10 commits intofeast-dev:masterfrom
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2429 +/- ##
==========================================
+ Coverage 83.66% 84.73% +1.06%
==========================================
Files 125 126 +1
Lines 10838 10811 -27
==========================================
+ Hits 9068 9161 +93
+ Misses 1770 1650 -120
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
felixwang9817
left a comment
felixwang9817
left a comment
There was a problem hiding this comment.
@pyalex some preliminary comments; will continue reviewing
pyalex
force-pushed
the
embedded-go
branch
2 times, most recently
from
1231226 to
301abe6
Compare
sdk/python/tests/integration/online_store/test_universal_online.py
Outdated
Show resolved
Hide resolved
felixwang9817
left a comment
felixwang9817
left a comment
There was a problem hiding this comment.
more comments, will continue looking
sdk/python/tests/integration/online_store/test_universal_online.py
Outdated
Show resolved
Hide resolved
go/embedded/online_features.go
Outdated
There was a problem hiding this comment.
Isn't this called immediately since we're returning in the next line? What's the value of deferring it?
There was a problem hiding this comment.
Some comments here would be nice
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
|
@achals @felixwang9817 Addressed your comments. |
achals
left a comment
achals
left a comment
There was a problem hiding this comment.
looks mostly fine, but the biggest piece is the featurestore.go file which I don't think I have confidence that I've reviewed thoroughly. I'll take a pass over that tomorrow morning.
Makefile
Outdated
| go mod tidy | ||
| go build -o ${ROOT_DIR}/sdk/python/feast/binaries/server github.com/feast-dev/feast/go/cmd/server | ||
| compile-go-lib: install-go-proto-dependencies install-go-ci-dependencies | ||
| python -m install pybindgen |
There was a problem hiding this comment.
should this be python -m pip install pybindgen? And should we pin the version of pybindgen?
There was a problem hiding this comment.
this is embarrassing) fixed
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
go/internal/feast/featurestore.go
Outdated
| currentVector.Values = builder.NewArray() | ||
| } else { | ||
| currentVector.Values = array.NewNull(numRows) | ||
| arrowValues, err := utils.ProtoValuesToArrowArray(protoValues, arrowAllocator, numRows) |
There was a problem hiding this comment.
nit, can we have a different module name instead of utils? Seems like it'll become a kitchen sink..
There was a problem hiding this comment.
renamed into types since it's type utils
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, pyalex The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: pyalex <[email protected]>
Signed-off-by: pyalex <[email protected]>
|
/lgtm |
7 participants
What this PR does / why we need it:
Converts Go-based implementation of get online features functionality into library which will be embedded into Python code and called directly from Python SDK (w/o GRPC in the middle). This allows us to also use Python functions as callbacks to call some implementations (like online/offline store connectors) from Go feature server.
Which issue(s) this PR fixes:
Fixes #