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: New PR - Updating dependencies so that feast can be run on 3.11. by lokeshrangineni · Pull Request #3993 · feast-dev/feast

Conversation

@lokeshrangineni

@lokeshrangineni lokeshrangineni commented Mar 7, 2024

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Sorry, something went wrong.

Uh oh!

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

All reactions
@lokeshrangineni
lokeshrangineni requested a review from whoahbot as a code owner March 7, 2024 02:04
@lokeshrangineni lokeshrangineni mentioned this pull request Mar 7, 2024
feat: Updating dependencies so that feast can be run on 3.11. #3985
Closed
tokoko
tokoko reviewed Mar 7, 2024
View reviewed changes
Comment thread setup.py Outdated
"types-tabulate",
"virtualenv<20.24.2",
"pandas>=1.4.3,<2; python_version < '3.9'",
"pandas>=1.4.3,<2",

@tokoko tokoko Mar 7, 2024

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.

@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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@lokeshrangineni lokeshrangineni Mar 7, 2024

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.

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.

Sorry, something went wrong.

Uh oh!

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

tokoko reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@tokoko tokoko Mar 7, 2024

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 good, thanks

Sorry, something went wrong.

Uh oh!

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

lokeshrangineni reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@jeremyary jeremyary added the ok-to-test label Mar 7, 2024
@lokeshrangineni
lokeshrangineni force-pushed the master branch from dcf4b7b to f17e9f6 Compare March 7, 2024 16:59
@lokeshrangineni
lokeshrangineni requested review from kevjumba and niklasvm as code owners March 7, 2024 16:59
whoahbot
whoahbot reviewed Mar 7, 2024
View reviewed changes
Comment thread sdk/python/feast/infra/materialization/contrib/bytewax/bytewax_materialization_dataflow.py

@whoahbot whoahbot Mar 7, 2024

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@tokoko tokoko Mar 7, 2024

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.

@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?

Sorry, something went wrong.

Uh oh!

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

All reactions

@whoahbot whoahbot Mar 7, 2024

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.

IIRC the test depends on S3, Redshift and DynamoDB, and uses AWS permissions from an EKS cluster to access those resources.

Sorry, something went wrong.

Uh oh!

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

All reactions

@jeremyary jeremyary Mar 8, 2024

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@lokeshrangineni lokeshrangineni Mar 12, 2024 •
edited
Loading

Uh oh!

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

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions
@lokeshrangineni lokeshrangineni mentioned this pull request Mar 12, 2024
feat: Dropping the support for python 3.8 version from feast #4010
Merged
@jeremyary jeremyary added the do-not-merge/hold label Mar 12, 2024
@lokeshrangineni lokeshrangineni closed this Mar 19, 2024
@lokeshrangineni
lokeshrangineni force-pushed the master branch from 6715af5 to a0f7472 Compare March 19, 2024 13:28
@tokoko tokoko mentioned this pull request Apr 15, 2024
chore: Omit bytewax deps and materialization engine #4098
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@whoahbot whoahbot whoahbot left review comments
@jeremyary jeremyary jeremyary left review comments
@tokoko tokoko tokoko left review comments
@niklasvm niklasvm Awaiting requested review from niklasvm
@kevjumba kevjumba Awaiting requested review from kevjumba

Uh oh!

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

Assignees

No one assigned

Labels

do-not-merge/hold ok-to-test

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.

4 participants

@lokeshrangineni @whoahbot @jeremyary @tokoko
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 01402520 — 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.