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: New PR - Updating dependencies so that feast can be run on 3.11. #3993

Closed
wants to merge 0 commits into from

Conversation

lokeshrangineni
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

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",
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good, thanks

Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

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.

None yet

4 participants