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 Rockset as an OnlineStore #3405

Merged

Conversation

danielin917
Copy link
Contributor

@danielin917 danielin917 commented Dec 19, 2022

This commit adds Rockset as a contributed online store. It implements the Update, Teardown, Read and Write apis using the Rockset python client.

Signed-off-by: Daniel Lin dan@rockset.com

What this PR does / why we need it:
This PR adds Rockset as an implemented OnlineFeatureStore

Which issue(s) this PR fixes:

Fixes #
N/A

@danielin917
Copy link
Contributor Author

Hello! Integrations tests have all passed for the online store. Did not add a local universal test to the Makefile since there is no docker image to use.

@danielin917
Copy link
Contributor Author

/assign @mavysavydav

@danielin917
Copy link
Contributor Author

👋 Just checking in to see if there is anything I should do to make this easier to review

@mavysavydav
Copy link
Collaborator

hi! Do you know who might have context on these changes who may be better suited to review? I also recently stepped down from my role as a maintainer so am not as involved in reviewing these for the time being. Apologies for the delay! If there's nobody you have in mind who might be a good fit for reviewing, I can pass this on to someone else in the maintainer team.

@danielin917
Copy link
Contributor Author

Not sure who the best person is, I think @felixwang9817 mentioned he was interested but not sure who else could review

@mavysavydav
Copy link
Collaborator

@felixwang9817 -- were you interested in taking a look at this or do you know who might be?

Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

looks good overall! just a few nits

docs/SUMMARY.md Outdated Show resolved Hide resolved
docs/reference/online-stores/README.md Outdated Show resolved Hide resolved
docs/reference/online-stores/rockset.md Outdated Show resolved Hide resolved
docs/reference/online-stores/rockset.md Outdated Show resolved Hide resolved
docs/reference/online-stores/rockset.md Outdated Show resolved Hide resolved
docs/reference/online-stores/rockset.md Show resolved Hide resolved
sdk/python/requirements/py3.8-ci-requirements.txt Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@felixwang9817
Copy link
Collaborator

hey @danielin917 thanks for the PR and sorry for the delay on the review - I'm happy to help you get this PR merged

I left a few nits above, and I think you'll want to rebase to ensure you can pass the latest tests!

@danielin917 danielin917 requested review from felixwang9817 and adchia and removed request for felixwang9817 January 18, 2023 21:45
@danielin917
Copy link
Contributor Author

danielin917 commented Jan 18, 2023

Thanks for all the comments! Reran make test-python-integration and don't see any Rockset errors

@danielin917 danielin917 requested review from felixwang9817 and removed request for adchia January 18, 2023 23:26
Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

/lgtm

@felixwang9817
Copy link
Collaborator

@danielin917 I stamped but looks like there's actually a few outstanding errors that need to be addressed

@danielin917
Copy link
Contributor Author

danielin917 commented Jan 24, 2023

ERROR: Cannot uninstall types-protobuf 4.21.0.3, RECORD file not found. You might be able to recover from this via: 'pip install --force-reinstall --no-deps types-protobuf==4.21.0.3'.

Not sure if there is a way to just rerun this test and this goes away or might need to play with dependencies more.

@felixwang9817
Copy link
Collaborator

ERROR: Cannot uninstall types-protobuf 4.21.0.3, RECORD file not found. You might be able to recover from this via: 'pip install --force-reinstall --no-deps types-protobuf==4.21.0.3'.

Not sure if there is a way to just rerun this test and this goes away or might need to play with dependancies more.

@danielin917 looks like it's probably just a dependencies issue - I'll look into it today/tmrw!

setup.py Outdated
@@ -151,7 +156,7 @@
"minio==7.1.0",
"mock==2.0.0",
"moto<4",
"mypy>=0.931",
"mypy>=0.931, <=0.981",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@danielin917 why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was seeing a lot of unrelated lint errors before from different files. But actually now after removing the cap lint passed so not sure what changed

setup.py Outdated
@@ -174,7 +179,7 @@
"assertpy==1.1",
"pip-tools",
"pybindgen",
"types-protobuf",
"types-protobuf<4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@danielin917 why is this necessary? the error message in the failing unit tests is ERROR: Cannot uninstall types-protobuf 4.21.0.3, RECORD file not found. You might be able to recover from this via: 'pip install --force-reinstall --no-deps types-protobuf==4.21.0.3'., so this line seems very suspicious to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey yea, mentioned above that I pinned this type because the linter was producing a lot of errors using the new version Library stubs not installed for "google.*". One thing that might be causing this is that we have:
"mypy-protobuf==3.1",

@danielin917
Copy link
Contributor Author

Not sure what happened between earlier testing and now but after removing cap on mypy and types-proto and this time I saw no lint errors locally

@danielin917 danielin917 force-pushed the add_rockset_as_online_store branch 2 times, most recently from 8bb7be7 to 5134577 Compare January 25, 2023 22:44
@danielin917
Copy link
Contributor Author

danielin917 commented Jan 26, 2023

Ugh, I am going to open a separate commit for upgrading mypy and deal with all of the linting issues there to unblock this diff as is instead of playing around with package dependencies

Looks like these were recently pinned? make lint-python works locally after rebase

This commit adds Rockset as a contributed online store. It implements the
Update, Teardown, Read and Write apis using the Rockset pythonn client.

Signed-off-by: Daniel Lin <dan@rockset.com>
@danielin917 danielin917 requested review from felixwang9817 and adchia and removed request for felixwang9817 and adchia February 7, 2023 21:10
@danielin917
Copy link
Contributor Author

Sorry to keep pinging just want to test the workflow after the rebase

Copy link
Collaborator

@felixwang9817 felixwang9817 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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielin917, felixwang9817

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

The pull request process is described here

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 fd91cda into feast-dev:master Feb 9, 2023
@felixwang9817
Copy link
Collaborator

@danielin917 we finally got this merged in!! thanks a ton for the contribution, and for your patience throughout the review process - I know we ran into a bunch of issues throughout 😅

@danielin917
Copy link
Contributor Author

Ahh sweet! Thanks @felixwang9817 and @adchia for the assist!

kevjumba pushed a commit that referenced this pull request Mar 17, 2023
# [0.30.0](v0.29.0...v0.30.0) (2023-03-17)

### Bug Fixes

* Add description attribute to the Field.from_proto method ([#3469](#3469)) ([473f8d9](473f8d9))
* Add filesystem kwargs when read prev_table on FileRetrievalJob (… ([#3491](#3491)) ([dca4745](dca4745)), closes [#3490](#3490)
* Feature view `entities` from_proto type ([#3524](#3524)) ([57bbb61](57bbb61))
* Fix missing requests requirement after GCP requirement removed. Make BigQuerySource not require gcp extra ([2c85421](2c85421))
* Fix SQL Registry cache miss ([#3482](#3482)) ([3249b97](3249b97))
* Fixed path inside quickstart notebook ([#3456](#3456)) ([66edc32](66edc32))
* Improve BQ point-in-time joining scalability ([#3429](#3429)) ([ff66784](ff66784))
* Pin typeguard to 2.13.3 which is what we are currently using. ([#3542](#3542)) ([61f6fb0](61f6fb0))
* Protobuf lower bound to 3.20 to alert that Feast is incompatible with tensorflow ([#3476](#3476)) ([9ca59e3](9ca59e3))
* Spark kafka processor sorting ([#3479](#3479)) ([f2cbf43](f2cbf43))
* UI working behind base url ([#3514](#3514)) ([9a3fd98](9a3fd98))
* Update go dependencies ([#3512](#3512)) ([bada97c](bada97c))

### Features

* Add Rockset as an OnlineStore ([#3405](#3405)) ([fd91cda](fd91cda))
* Add Snowflake Registry ([#3363](#3363)) ([ec1e61d](ec1e61d))
* Adding query timeout to `to_df` and `to_arrow` retrieval methods ([#3505](#3505)) ([bab6644](bab6644))
* adds k8s config options to Bytewax materialization engine ([#3518](#3518)) ([1883f55](1883f55))
achals pushed a commit that referenced this pull request Mar 24, 2023
# [0.30.0](v0.29.0...v0.30.0) (2023-03-24)

### Bug Fixes

* Add description attribute to the Field.from_proto method ([#3469](#3469)) ([473f8d9](473f8d9))
* Add filesystem kwargs when read prev_table on FileRetrievalJob (… ([#3491](#3491)) ([dca4745](dca4745)), closes [#3490](#3490)
* Bytewax image pull secret config ([#3547](#3547)) ([d2d13b1](d2d13b1))
* Clean up Rockset Online Store for use ([#3549](#3549)) ([a76c6d0](a76c6d0))
* Feature view `entities` from_proto type ([#3524](#3524)) ([57bbb61](57bbb61))
* Fix missing requests requirement after GCP requirement removed. Make BigQuerySource not require gcp extra ([2c85421](2c85421))
* Fix SQL Registry cache miss ([#3482](#3482)) ([3249b97](3249b97))
* Fixed path inside quickstart notebook ([#3456](#3456)) ([66edc32](66edc32))
* Improve BQ point-in-time joining scalability ([#3429](#3429)) ([ff66784](ff66784))
* Pin typeguard to 2.13.3 which is what we are currently using. ([#3542](#3542)) ([61f6fb0](61f6fb0))
* Protobuf lower bound to 3.20 to alert that Feast is incompatible with tensorflow ([#3476](#3476)) ([9ca59e3](9ca59e3))
* Spark kafka processor sorting ([#3479](#3479)) ([f2cbf43](f2cbf43))
* UI working behind base url ([#3514](#3514)) ([9a3fd98](9a3fd98))
* Update go dependencies ([#3512](#3512)) ([bada97c](bada97c))

### Features

* Add Rockset as an OnlineStore ([#3405](#3405)) ([fd91cda](fd91cda))
* Add Snowflake Registry ([#3363](#3363)) ([ec1e61d](ec1e61d))
* Added SnowflakeConnection caching ([#3531](#3531)) ([f9f8df2](f9f8df2))
* Adding query timeout to `to_df` and `to_arrow` retrieval methods ([#3505](#3505)) ([bab6644](bab6644))
* adds k8s config options to Bytewax materialization engine ([#3518](#3518)) ([1883f55](1883f55))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants