-
Notifications
You must be signed in to change notification settings - Fork 996
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
feat: Add Rockset as an OnlineStore #3405
Conversation
ac2da51
to
2b6c591
Compare
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. |
/assign @mavysavydav |
👋 Just checking in to see if there is anything I should do to make this easier to review |
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. |
Not sure who the best person is, I think @felixwang9817 mentioned he was interested but not sure who else could review |
@felixwang9817 -- were you interested in taking a look at this or do you know who might be? |
There was a problem hiding this 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
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! |
2b6c591
to
4756df4
Compare
Thanks for all the comments! Reran make test-python-integration and don't see any Rockset errors |
4756df4
to
af7b49a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@danielin917 I stamped but looks like there's actually a few outstanding errors that need to be addressed |
af7b49a
to
490d723
Compare
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. |
@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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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",
f93458b
to
3b66a2e
Compare
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 |
8bb7be7
to
5134577
Compare
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>
5134577
to
a799056
Compare
Sorry to keep pinging just want to test the workflow after the rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[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 |
@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 😅 |
Ahh sweet! Thanks @felixwang9817 and @adchia for the assist! |
# [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))
# [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))
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