-
Notifications
You must be signed in to change notification settings - Fork 478
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
Instances of the FeatureService
s are now used instead of only the names of the FeatureServices.
#3209
Instances of the FeatureService
s are now used instead of only the names of the FeatureServices.
#3209
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
In addition to the FeatureService, there are other implementations that do not return/use objects, but instead use names. (e.g. |
I'm not sure how tests should be written here at zenml. In the case of feast integration, you would have to do quite a bit of mocking. |
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, assuming tests all pass...
@strickvl When I start the tests locally with the bash script, I get the following error. RuntimeError: The ZenML database has never been migrated with alembic before. This can happen if you are performing a direct upgrade from a really old version of ZenML. This direct upgrade path is not supported anymore. Please upgrade your ZenML installation first to 0.54.0 or an earlier version and then
to the latest version. I don't really understand this error. Since I have not added any new tests and have not found any tests for the feast integration, everything should probably still work. |
3509ca8
to
74a7f2f
Compare
@aiakide This is probably because the tests are trying to run with an already existing, very old database. Which OS are you running on? There should be a |
74a7f2f
to
90b3733
Compare
Yes, that helped. Thanks for that :) |
90b3733
to
fd74a2c
Compare
@aiakide Is this ready to merge from your side or are you still making changes? |
@schustmi Yes, I'm done. Can be merged. 🚀 |
Describe changes
I have adapted the
get_feature_services
function so that it returns instances of theFeaturesService
class and not just a list of feature service names. In addition, the functionsget_online_features
andget_historical_features
of the integration can now be used with a list of features / feature views as well as with aFeatureService
instance.This brings the implementation of the feast integration closer to the feast API.
I have also set the minimum version of feast to 0.12.0 to ensure that the necessary Pythom API is available from feast.
This PR closes #3180.
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes