-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: New PR - Updating dependencies so that feast can be run on 3.11. #3993
Conversation
setup.py
Outdated
@@ -194,7 +194,7 @@ | |||
"types-setuptools", | |||
"types-tabulate", | |||
"virtualenv<20.24.2", | |||
"pandas>=1.4.3,<2; python_version < '3.9'", | |||
"pandas>=1.4.3,<2", |
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.
@lokeshrangineni sorry, i meant that you can remove the line completely, not just the version specifier. pandas is already listed above, this was just to override it for 3.8 ci tests.
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.
Sorry @tokoko, I am a beginner to python so forgive my understanding. I have updated the PR again with the updated feedback. Please review it again.
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, thanks
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 putting this PR together.
There are a number of API changes between Bytewax 0.15.1 and 0.18.2 that would need to be addressed.
I took at stab at rewriting this dataflow using the 0.18.2 API here: gist, but I may need some help testing these changes as I don't currently have a working Feast materialization environment.
One other thing to consider is that we would need to coordinate building a new docker image that will be used for materialization. The version referenced in the documentation is unfortunately using the latest
tag, so existing materialization that use that tag would break when publishing a new version of the image. I'm happy to coordinate building and pushing that image to our docker hub account when we get things sorted.
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.
@whoahbot thanks for pointing that out. How can we test it better? I saw that the existing integration test depends on eks, is there any specific reason for that? Can we rewrite it to work on minikube or kind for example?
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.
IIRC the test depends on S3, Redshift and DynamoDB, and uses AWS permissions from an EKS cluster to access those resources.
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.
First off, thanks for jumping back in with us @whoahbot! Tackling all the breaking changes we'll need to step through is sure to be easier with your help. @tokoko was just discussing over in Slack with us the possibility of splitting the PR into 2 efforts - one for dropping 3.8 and another for adding 3.11 so that we can give the latter proper testing. If things start shuffling without discussion here, that's probably why.
I'm sure you stay covered up with work, but if you find a chance to swing through Slack or catch a community call, would be great to say hello and introduce some new faces.
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.
After the conversation to split up this PR in to two separate tasks for better manageability - I have created the PR|4010 to drop the Python 3.8 support.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #