Skip to content

Comments

chore: Fixes and Readme for python<>go interface#2936

Merged
feast-ci-bot merged 10 commits intofeast-dev:masterfrom
achals:acha/go-wip
Jul 15, 2022
Merged

chore: Fixes and Readme for python<>go interface#2936
feast-ci-bot merged 10 commits intofeast-dev:masterfrom
achals:acha/go-wip

Conversation

@achals
Copy link
Member

@achals achals commented Jul 13, 2022

Signed-off-by: Achal Shah [email protected]

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022 •
edited
Loading

Codecov Report

Merging #2936 (78b046d) into master (054446c) will decrease coverage by 19.22%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #2936       +/-   ##
===========================================
- Coverage   78.57%   59.35%   -19.23%     
===========================================
  Files         179      183        +4     
  Lines       15877    16119      +242     
===========================================
- Hits        12475     9567     -2908     
- Misses       3402     6552     +3150
Flag Coverage Δ
integrationtests ?
unittests 59.35% <0.00%> (-0.29%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ython/feast/embedded_go/online_features_service.py 27.81% <0.00%> (-0.22%) ⬇️
.../integration/online_store/test_online_retrieval.py 16.84% <0.00%> (-83.16%) ⬇️
sdk/python/tests/utils/online_read_write_test.py 19.35% <0.00%> (-80.65%) ⬇️
...fline_store/test_universal_historical_retrieval.py 23.36% <0.00%> (-76.64%) ⬇️
...ts/integration/offline_store/test_offline_write.py 27.69% <0.00%> (-72.31%) ⬇️
...dk/python/tests/integration/e2e/test_validation.py 27.77% <0.00%> (-71.43%) ⬇️
sdk/python/feast/wait.py 23.52% <0.00%> (-70.59%) ⬇️
...ests/integration/e2e/test_python_feature_server.py 29.50% <0.00%> (-70.50%) ⬇️
...ion/registration/test_stream_feature_view_apply.py 28.84% <0.00%> (-69.24%) ⬇️
...gration/registration/test_feature_service_apply.py 31.25% <0.00%> (-68.75%) ⬇️
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 054446c...78b046d. Read the comment docs.

kevjumba
kevjumba reviewed Jul 13, 2022
View reviewed changes
Copy link
Collaborator

@kevjumba kevjumba left a comment

Choose a reason for hiding this comment

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

@achals I think in online_features_service.py right after resp = record_batch_to_online_response(record_batch) we should also explicilty release the returned recordbatch. I'm not quite sure if returning from getOnlineFeatures actually decrements the reference counter but better safe than sorry?

@achals
Copy link
Member Author

achals commented Jul 13, 2022

@achals I think in online_features_service.py right after resp = record_batch_to_online_response(record_batch) we should also explicilty release the returned recordbatch. I'm not quite sure if returning from getOnlineFeatures actually decrements the reference counter but better safe than sorry?

I think we can explicitly del it

achals and others added 5 commits July 13, 2022 14:26
@achals @felixwang9817
chore: Fixes and Readme for python<>go interface
a5e270d 
Signed-off-by: Achal Shah <[email protected]>
@achals @felixwang9817
cr
492dc91 
Signed-off-by: Achal Shah <[email protected]>
@felixwang9817
Switch to cgo allocator
f205e41 
Signed-off-by: Felix Wang <[email protected]>
@felixwang9817
Fix memory leak
ae71efa 
Signed-off-by: Felix Wang <[email protected]>
@felixwang9817
Fix another memory leak
a6232d9 
Signed-off-by: Felix Wang <[email protected]>
achals
achals commented Jul 14, 2022
View reviewed changes
go/internal/feast/featurestore.go

result := make([]*onlineserving.FeatureVector, 0)
arrowMemory := memory.NewGoAllocator()
arrowMemory := memory.NewCgoArrowAllocator()
Copy link
Member Author

@achals achals Jul 14, 2022

Choose a reason for hiding this comment

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

We should fix this these lint issues

Copy link
Collaborator

@felixwang9817 felixwang9817 Jul 14, 2022

Choose a reason for hiding this comment

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

I think #2940 should resolve this?

achals
achals commented Jul 14, 2022
View reviewed changes
go/internal/feast/server/logging/memorybuffer_test.go Outdated
assert.Nil(t, err)
for colIdx := 0; colIdx < int(record.NumCols()); colIdx++ {
assert.Equal(t, expectedRecord.Column(colIdx), record.Column(colIdx), "Columns with idx %d are not equal", colIdx)
assert.Equal(t, true, array.Equal(expectedRecord.Column(colIdx), record.Column(colIdx)), "Columns with idx %d are not equal", colIdx)
Copy link
Member Author

@achals achals Jul 14, 2022

Choose a reason for hiding this comment

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

https://pkg.go.dev/github.com/stretchr/testify/assert#True instead of assert.Equal

Copy link
Collaborator

@felixwang9817 felixwang9817 Jul 14, 2022

Choose a reason for hiding this comment

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

fixed

This was referenced Jul 15, 2022
Closed
Closed
kevjumba
kevjumba approved these changes Jul 15, 2022
View reviewed changes
Copy link
Collaborator

@kevjumba kevjumba left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

feast-ci-bot commented Jul 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, kevjumba

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit 268f28d into feast-dev:master Jul 15, 2022
@achals achals deleted the acha/go-wip branch July 15, 2022 03:54
adchia pushed a commit that referenced this pull request Jul 15, 2022
@achals @felixwang9817 @adchia
chore: Fixes and Readme for python<>go interface (#2936)
a30bf86 
* chore: Fixes and Readme for python<>go interface

Signed-off-by: Achal Shah <[email protected]>

* cr

Signed-off-by: Achal Shah <[email protected]>

* Switch to cgo allocator

Signed-off-by: Felix Wang <[email protected]>

* Fix memory leak

Signed-off-by: Felix Wang <[email protected]>

* Fix another memory leak

Signed-off-by: Felix Wang <[email protected]>

* Do not use cgo allocator for memory buffers

Signed-off-by: Felix Wang <[email protected]>

* Switch to cgo allocator for memory buffers

Signed-off-by: Felix Wang <[email protected]>

* Switch test to pass

Signed-off-by: Felix Wang <[email protected]>

* Use more idiomatic way to test truth

Signed-off-by: Felix Wang <[email protected]>

* Update docs

Signed-off-by: Felix Wang <[email protected]>

Co-authored-by: Felix Wang <[email protected]>
kevjumba pushed a commit that referenced this pull request Jul 19, 2022
@achals @felixwang9817 @kevjumba
chore: Fixes and Readme for python<>go interface (#2936)
922dcd3 
* chore: Fixes and Readme for python<>go interface

Signed-off-by: Achal Shah <[email protected]>

* cr

Signed-off-by: Achal Shah <[email protected]>

* Switch to cgo allocator

Signed-off-by: Felix Wang <[email protected]>

* Fix memory leak

Signed-off-by: Felix Wang <[email protected]>

* Fix another memory leak

Signed-off-by: Felix Wang <[email protected]>

* Do not use cgo allocator for memory buffers

Signed-off-by: Felix Wang <[email protected]>

* Switch to cgo allocator for memory buffers

Signed-off-by: Felix Wang <[email protected]>

* Switch test to pass

Signed-off-by: Felix Wang <[email protected]>

* Use more idiomatic way to test truth

Signed-off-by: Felix Wang <[email protected]>

* Update docs

Signed-off-by: Felix Wang <[email protected]>

Co-authored-by: Felix Wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixwang9817 felixwang9817 felixwang9817 left review comments

@kevjumba kevjumba kevjumba approved these changes

Assignees

@kevjumba kevjumba

Labels

approved lgtm size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@achals @codecov-commenter @feast-ci-bot @kevjumba @felixwang9817