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

Closed
wants to merge 4 commits into from

Conversation

lokeshrangineni
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Signed-off-by: Lokesh Rangineni <lokeshforjava@gmail.com>
@lokeshrangineni lokeshrangineni changed the title Updating dependencies so that feast can be run on 3.11. feast: Updating dependencies so that feast can be run on 3.11. Mar 5, 2024
@jeremyary
Copy link
Collaborator

@lokeshrangineni thanks for giving this another look! PTAL at the linter output, but I'm going to go ahead and approve tests so we can get an idea how integration results look 👍

@lokeshrangineni lokeshrangineni changed the title feast: Updating dependencies so that feast can be run on 3.11. Feat: Updating dependencies so that feast can be run on 3.11. Mar 5, 2024
@lokeshrangineni lokeshrangineni changed the title Feat: Updating dependencies so that feast can be run on 3.11. feat: Updating dependencies so that feast can be run on 3.11. Mar 5, 2024
@jeremyary
Copy link
Collaborator

copying out the two error lines from lint job so we don't have to keep pulling context back up

feast/infra/materialization/contrib/bytewax/bytewax_materialization_dataflow.py:9: error: Module "bytewax.inputs" has no attribute "ManualInputConfig"  [attr-defined]
feast/infra/materialization/contrib/bytewax/bytewax_materialization_dataflow.py:10: error: Module "bytewax.outputs" has no attribute "ManualOutputConfig"  [attr-defined]

setup.py Outdated
@@ -187,7 +187,7 @@
"types-setuptools",
"types-tabulate",
"virtualenv<20.24.2",
"pandas>=1.4.3,<2; python_version < '3.9'",
"pandas>=1.4.3,<2; python_version <= '3.11'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get this change. This rule was added during #3957 specifically to handle the issue with the latest pandas version for 3.8 which contained a bug in pandas.testing. If we're dropping 3.8 from ci, this rule can be removed.

@tokoko
Copy link
Collaborator

tokoko commented Mar 6, 2024

Are we dropping ci, but keeping support? shouldn't this PR change REQUIRES_PYTHON as well?

@jeremyary jeremyary self-requested a review March 6, 2024 17:38
@jeremyary
Copy link
Collaborator

@tokoko please keep 'em coming, I'm just trying to work thru the required check, but I'm missing historical context. Appreciate the good feedback!

@tokoko
Copy link
Collaborator

tokoko commented Mar 6, 2024

@jeremyary no problem :) I think we should definitely be running ci for the lowest version of python supported. Without that, we would very easily introduce changes and dependencies that would break the baseline version. that's just my opinion. The larger question here is whether we want to drop 3.8 support or not. I'm all for dropping it (and I kinda feel bad about stopping this PR :D ) but I think we should also agree on removal policy before doing that. We don't have anything documented and 3.8 still hasn't reached EOL. I posted on slack a couple weeks back suggesting to adopt NEP-29 instead of following official python deprecations. As much as I'd like to see 3.8 gone, there's probably a discussion to be had before doing that.

@lokeshrangineni
Copy link
Contributor Author

@jeremyary no problem :) I think we should definitely be running ci for the lowest version of python supported. Without that, we would very easily introduce changes and dependencies that would break the baseline version. that's just my opinion. The larger question here is whether we want to drop 3.8 support or not. I'm all for dropping it (and I kinda feel bad about stopping this PR :D ) but I think we should also agree on removal policy before doing that. We don't have anything documented and 3.8 still hasn't reached EOL. I posted on slack a couple weeks back suggesting to adopt NEP-29 instead of following official python deprecations. As much as I'd like to see 3.8 gone, there's probably a discussion to be had before doing that.

Thank you for the context. I am going to update the PR with the information you provided.

@jeremyary
Copy link
Collaborator

jeremyary commented Mar 6, 2024

@jeremyary no problem :) I think we should definitely be running ci for the lowest version of python supported. Without that, we would very easily introduce changes and dependencies that would break the baseline version. that's just my opinion. The larger question here is whether we want to drop 3.8 support or not. I'm all for dropping it (and I kinda feel bad about stopping this PR :D ) but I think we should also agree on removal policy before doing that. We don't have anything documented and 3.8 still hasn't reached EOL. I posted on slack a couple weeks back suggesting to adopt NEP-29 instead of following official python deprecations. As much as I'd like to see 3.8 gone, there's probably a discussion to be had before doing that.

thanks again. A related thread's been going through our last couple of comm calls, though we didn't get into the procedural side. Without moving on from 3.8 we're stuck in a bit of a hard place around other dependencies that will soon stop supporting 3.8. I'm down for a collective rehashing via Slack if warranted, though cc @lokeshrangineni @etirelli @franciscojavierarceo

…he upgrade. These classes ManualInputConfig and ManualOutputConfig have been removed from the new versions. hope this fixes the lint errors.

Signed-off-by: Lokesh Rangineni <lokeshforjava@gmail.com>

Signed-off-by: Lokesh Rangineni <lokeshforjava@gmail.com>
…tly build to the lowest python version we are planning to support. Updating the REQUIRES_PYTHON to 3.9 as well.

Signed-off-by: Lokesh Rangineni <lokeshforjava@gmail.com>

Signed-off-by: Lokesh Rangineni <lokeshforjava@gmail.com>
@lokeshrangineni
Copy link
Contributor Author

I have closed the this PR and opened the PR to test if the unit test cases job to run using python 3.8 have been removed or not.

Please find the new PR here - #3993

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

3 participants