GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: Add Cassandra/AstraDB online store contribution by hemidactylus · Pull Request #2873 · feast-dev/feast

Conversation

@hemidactylus

@hemidactylus hemidactylus commented Jun 28, 2022 •
edited by adchia
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR adds support for Cassandra/Astra DB online store for Feast.

What this PR does / why we need it:
Self-managed Cassandra clusters and Astra DB instances (based on Apache Cassandra) are a good fit to work as online features stores. This plugin makes it possible to do so within Feast.

Notes:

  • make test-python: all passed.
  • The documentation is up to date, with a README.md in the module dir and the relevant rst autodoc files.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@feast-ci-bot feast-ci-bot added the size/XXL label Jun 28, 2022
@hemidactylus
hemidactylus marked this pull request as draft June 28, 2022 09:52
@feast-ci-bot feast-ci-bot added the do-not-merge/work-in-progress label Jun 28, 2022
@codecov-commenter

codecov-commenter commented Jun 28, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Codecov Report

Merging #2873 (6cd1e90) into master (63d541d) will increase coverage by 9.02%.
The diff coverage is 11.01%.

@@            Coverage Diff             @@
##           master    #2873      +/-   ##
==========================================
+ Coverage   67.64%   76.67%   +9.02%     
==========================================
  Files         167      198      +31     
  Lines       14696    16344    +1648     
==========================================
+ Hits         9941    12531    +2590     
+ Misses       4755     3813     -942
Flag Coverage Δ
integrationtests 67.44% <18.18%> (-0.20%) ⬇️
unittests 58.61% <11.01%> (?)

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

Impacted Files Coverage Δ
sdk/python/feast/cli.py 41.69% <ø> (ø)
sdk/python/feast/repo_config.py 88.80% <ø> (+5.40%) ⬆️
setup.py 0.00% <0.00%> (ø)
...b/cassandra_online_store/cassandra_online_store.py 2.63% <2.63%> (ø)
sdk/python/feast/file_utils.py 40.00% <40.00%> (ø)
.../feature_repos/universal/online_store/cassandra.py 42.85% <42.85%> (ø)
...ine_stores/contrib/cassandra_repo_configuration.py 100.00% <100.00%> (ø)
sdk/python/feast/repo_operations.py 51.70% <100.00%> (+26.94%) ⬆️
...s/contrib/trino_offline_store/tests/data_source.py 17.64% <0.00%> (ø)
...ffline_stores/contrib/trino_offline_store/trino.py 8.72% <0.00%> (ø)
... and 98 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
achals
achals reviewed Jun 30, 2022
View reviewed changes

@achals achals left a comment

Copy link
Copy Markdown
Member

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.

Thanks for the PR @hemidactylus ! This generally looks good to me. one thing that I would add is to add the spec for how data is stored in C* or Astra here: https://github.com/feast-dev/feast/tree/master/docs/specs

Other than that, I think the main gap is in testing. What kind of support do you need there to get this over the line?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Outdated

@achals achals Jun 30, 2022

Copy link
Copy Markdown
Member

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.

Curious, what effect does preparing sessions have here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hemidactylus hemidactylus Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

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.

Preparing statements means that the "shape" of the statement is exchanged once between client and DB servers and metadata info (schema, types) is cached on both sides under an ID to be re-used later any time the command is sent to the DB (read or write), reducing the parsing-time overhead basically. (See https://docs.datastax.com/en/developer/python-driver/3.25/getting_started/#prepared-statement if you are interested.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kevjumba and achals reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Outdated
Comment on lines 58 to 63

@achals achals Jun 30, 2022

Copy link
Copy Markdown
Member

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.

We're in the process of deprecating created_ts in other sources so I would be tempted to keep this out of the implementation for now if feasible..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hemidactylus hemidactylus Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

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.

All right, I'll be ignoring created_ts even if non-null passed within data to function online_write_batch.
In other words, I'll pretend created_ts is always None and go from there.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Outdated

@achals achals Jun 30, 2022

Copy link
Copy Markdown
Member

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.

Just a heads up that there's a bug (#2345) in the implementation of this function right now that we're trying to fix (#2609).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
hemidactylus reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@achals achals Jul 21, 2022

Copy link
Copy Markdown
Member

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.

same comment here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@hemidactylus

hemidactylus commented Jul 7, 2022

Copy link
Copy Markdown
Contributor Author

I added the specifications for this online store in /docs/specs/online_store_format.md and polished a couple of things. The only missing part is testing. I'll try to reach out on the Feast slack about that. Thank you so much!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hemidactylus

hemidactylus commented Jul 11, 2022

Copy link
Copy Markdown
Contributor Author

The Cassandra plugin now is covered by the universal online-store tests with Dockerized local cassandra instance. (Thanks to the Feast team for precious guidance!)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hemidactylus
hemidactylus requested a review from achals July 11, 2022 07:23
@adchia
adchia marked this pull request as ready for review July 11, 2022 13:53
@feast-ci-bot feast-ci-bot removed the do-not-merge/work-in-progress label Jul 11, 2022
@adchia

adchia commented Jul 11, 2022

Copy link
Copy Markdown
Collaborator

you'll need to also sign your commits too!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@achals achals self-assigned this Jul 11, 2022
@achals

achals commented Jul 11, 2022

Copy link
Copy Markdown
Member

@hemidactylus thanks for your hard work! I'll be reviewing this today. But like @adchia mentioned do you mind signing your commits?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hemidactylus

hemidactylus commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

Hello, I should have signed the commit. Thank you for your support and sorry for the inconvenience!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kevjumba kevjumba added the ok-to-test label Jul 19, 2022
kevjumba
kevjumba reviewed Jul 19, 2022
View reviewed changes

@kevjumba kevjumba left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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 great! We still need to add some unit tests I think for the online read and write functionality. @hemidactylus Also, need to fix the integration tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Outdated

@kevjumba kevjumba Jul 19, 2022

Copy link
Copy Markdown
Collaborator

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.

Would rather you name this featureView to prevent confusion

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
achals reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@hemidactylus hemidactylus Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

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 am a bit reluctant to rename ths parameter other than table: it has this name on any other online store implementation, plus in several places in the test code there are invocations to the method that use table for keyword-arguments.
Wouldn't that break some tests (or worse) ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/templates/cassandra/bootstrap.py Outdated

@kevjumba kevjumba Jul 19, 2022

Copy link
Copy Markdown
Collaborator

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.

can you move these to utils so we can use them for other use cases too?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hemidactylus hemidactylus Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

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.

done (this led me to create a top-level "file_utils.py' module and touch several template-related modules as well).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
achals
achals approved these changes Jul 21, 2022
View reviewed changes

@achals achals left a comment

Copy link
Copy Markdown
Member

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.

A few last nits, and a rebase is probably necessary (which may reformat all the python files), but after that I think we're ready to merge!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/tests/integration/feature_repos/universal/online_store/cassandra.py Outdated
Comment on lines 30 to 34

@achals achals Jul 13, 2022

Copy link
Copy Markdown
Member

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.

nit, we usually prefer f-strings in the codebase these days.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hemidactylus hemidactylus Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

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.

Sure, thank you for pointing out.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Outdated

@achals achals Jul 21, 2022

Copy link
Copy Markdown
Member

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.

Strongly recommend using entity_key_serialization_version=2 in serialize_entity_key.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hemidactylus hemidactylus Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

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.

Yep, I'm glad you noticed that!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread sdk/python/feast/infra/online_stores/contrib/cassandra_online_store/cassandra_online_store.py Outdated

@achals achals Jul 21, 2022

Copy link
Copy Markdown
Member

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.

same comment here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@feast-ci-bot feast-ci-bot added the approved label Jul 21, 2022
@adchia

adchia commented Aug 6, 2022

Copy link
Copy Markdown
Collaborator

hey! wanted to double check here and make sure things were still going well?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hemidactylus

hemidactylus commented Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

hey! wanted to double check here and make sure things were still going well?

Hello! I was OOTO for some time, sorry. I got back to working on this PR right now, thank you for the feedback!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kevjumba kevjumba added ok-to-test and removed ok-to-test labels Aug 8, 2022
@kevjumba

kevjumba commented Aug 8, 2022

Copy link
Copy Markdown
Collaborator

@hemidactylus The PR looks pretty good but there is something about the way you setup the test environment that is causing the pr integration tests to hang. Could you please take a look at that?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hemidactylus

hemidactylus commented Aug 9, 2022

Copy link
Copy Markdown
Contributor Author

there is something about the way you setup the test environment that is causing the pr integration tests to hang. Could you please take a look at that?

Thank you for spotting that. Fixed now.
More generally, I am almost done adding dedicated test targets in the Makefile, it should all be done at that point.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kevjumba and others added 3 commits August 9, 2022 14:24
@kevjumba @sl-at-ibm
Cassandra online store
02ac46e 
* Refactor file-editing to a shared utils module
* Use f-strings in the CassandraOnlineStoreCreator
* Specify version 2 in serializing to make the entity key
* Remove unnecessary empty comment lines
* Rename proj to columns in _read_rows_by_entity_key
* Introduce Cassandra-specific pytest targets
* Adapt roadmaps and docs to cover/index Cassandra online store
* Add license notes to code files

Signed-off-by: Stefano Lottini <[email protected]>
@adchia
remove from main CI path and update entity key serialization in template
4c18d45 
Signed-off-by: Danny Chiao <[email protected]>
@adchia
revert makefile change
6cd1e90 
Signed-off-by: Danny Chiao <[email protected]>
adchia
adchia approved these changes Aug 9, 2022
View reviewed changes

@adchia adchia left a comment

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@feast-ci-bot feast-ci-bot assigned adchia Aug 9, 2022
@feast-ci-bot feast-ci-bot added the lgtm label Aug 9, 2022
@feast-ci-bot

feast-ci-bot commented Aug 9, 2022

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, adchia, hemidactylus

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:
  • OWNERS [achals,adchia]

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@feast-ci-bot
feast-ci-bot merged commit feb6cb8 into feast-dev:master Aug 9, 2022
kevjumba pushed a commit that referenced this pull request Aug 25, 2022
@feast-ci-bot
chore(release): release 0.24.0
c1c71da 
# [0.24.0](v0.23.0...v0.24.0) (2022-08-25)

### Bug Fixes

* Check if on_demand_feature_views is an empty list rather than None for snowflake provider ([#3046](#3046)) ([9b05e65](9b05e65))
* FeatureStore.apply applies BatchFeatureView correctly ([#3098](#3098)) ([41be511](41be511))
* Fix Feast Java inconsistency with int64 serialization vs python ([#3031](#3031)) ([4bba787](4bba787))
* Fix feature service inference logic ([#3089](#3089)) ([4310ed7](4310ed7))
* Fix field mapping logic during feature inference ([#3067](#3067)) ([cdfa761](cdfa761))
* Fix incorrect on demand feature view diffing and improve Java tests ([#3074](#3074)) ([0702310](0702310))
* Fix Java helm charts to work with refactored logic. Fix FTS image ([#3105](#3105)) ([2b493e0](2b493e0))
* Fix on demand feature view output in feast plan + Web UI crash ([#3057](#3057)) ([bfae6ac](bfae6ac))
* Fix release workflow to release 0.24.0 ([#3138](#3138)) ([a69aaae](a69aaae))
* Fix Spark offline store type conversion to arrow ([#3071](#3071)) ([b26566d](b26566d))
* Fixing Web UI, which fails for the SQL registry ([#3028](#3028)) ([64603b6](64603b6))
* Force Snowflake Session to Timezone UTC ([#3083](#3083)) ([9f221e6](9f221e6))
* Make infer dummy entity join key idempotent ([#3115](#3115)) ([1f5b1e0](1f5b1e0))
* More explicit error messages ([#2708](#2708)) ([e4d7afd](e4d7afd))
* Parse inline data sources ([#3036](#3036)) ([c7ba370](c7ba370))
* Prevent overwriting existing file during `persist` ([#3088](#3088)) ([69af21f](69af21f))
* Register BatchFeatureView in feature repos correctly ([#3092](#3092)) ([b8e39ea](b8e39ea))
* Return an empty infra object from sql registry when it doesn't exist ([#3022](#3022)) ([8ba87d1](8ba87d1))
* Teardown tables for Snowflake Materialization testing ([#3106](#3106)) ([0a0c974](0a0c974))
* UI error when saved dataset is present in registry. ([#3124](#3124)) ([83cf753](83cf753))
* Update sql.py ([#3096](#3096)) ([2646a86](2646a86))
* Updated snowflake template ([#3130](#3130)) ([f0594e1](f0594e1))

### Features

* Add authentication option for snowflake connector ([#3039](#3039)) ([74c75f1](74c75f1))
* Add Cassandra/AstraDB online store contribution ([#2873](#2873)) ([feb6cb8](feb6cb8))
* Add Snowflake materialization engine ([#2948](#2948)) ([f3b522b](f3b522b))
* Adding saved dataset capabilities for Postgres  ([#3070](#3070)) ([d3253c3](d3253c3))
* Allow passing repo config path via flag ([#3077](#3077)) ([0d2d951](0d2d951))
* Contrib azure provider with synapse/mssql offline store and Azure registry store ([#3072](#3072)) ([9f7e557](9f7e557))
* Custom Docker image for Bytewax batch materialization ([#3099](#3099)) ([cdd1b07](cdd1b07))
* Feast AWS Athena offline store (again) ([#3044](#3044)) ([989ce08](989ce08))
* Implement spark offline store `offline_write_batch` method ([#3076](#3076)) ([5b0cc87](5b0cc87))
* Initial Bytewax materialization engine ([#2974](#2974)) ([55c61f9](55c61f9))
* Refactor feature server helm charts to allow passing feature_store.yaml in environment variables ([#3113](#3113)) ([85ee789](85ee789))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevjumba kevjumba kevjumba left review comments
@achals achals achals approved these changes
@adchia adchia adchia approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@achals achals

@adchia adchia

Labels

approved lgtm ok-to-test size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@hemidactylus @codecov-commenter @adchia @achals @kevjumba @feast-ci-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.