-
Notifications
You must be signed in to change notification settings - Fork 87
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
Drop support for python 3.8 #4414
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4414 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 357 357
Lines 39965 39973 +8
=======================================
+ Hits 39840 39849 +9
+ Misses 125 124 -1 ☔ View full report in Codecov by Sentry. |
From #4314, here's the list of ones mentioned that are / have been unpinned and don't seem to cause any issues:
Here's the list of ones that cause issues (test failures):
Also, |
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.
You may need to remove the 3.8 workflows on the github side in order to merge
docs/source/release_notes.rst
Outdated
@@ -5,6 +5,7 @@ Release Notes | |||
* Reformatted files with updated black version :pr:`4395` | |||
* Fixes | |||
* Changes | |||
* Drop support for Python 3.8 :pr:`4414` |
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.
Mega nit: drop -> dropped :D
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.
Also, we should probably consider this a breaking change!
matplotlib-inline==0.1.7 | ||
networkx==3.1 | ||
nlp-primitives==2.11.0 |
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.
Why did nlp-primitives disappear?
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.
If I remember correctly, latest_dependency_checker
was not happy having that in there and would fail. I'll try adding it back again and seeing what happens.
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.
yeah it failed again when I re-added nlp-primitives
back into latest-dependency
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.
@MichaelFu512 I think you might want to try to get to the bottom of this. I think the NaturalLanguageFeaturizer
depends on nlp-primitives for the PolarityScore
and DiversityScore
primitives, so it seems like nlp-primitives
should be included here.
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.
I'm not up to date on how the dependency checkers are working in EvalML, but the latest version of nlp-primitives is 2.13.0
which was released yesterday and will be installed in your CI runs. Maybe you need to update the version from 2.11.0
to 2.13.0
here instead of removing the line?
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.
Same error weirdly enough,
Displaying dependencies which have changed, with main on the left and the new branch on the right:
21d20
< nlp-primitives==2.13.0
Normally, if the dependency was out of date, it'd look something like:
Displaying dependencies which have changed, with main on the left and the new branch on the right:
18c18
< matplotlib==3.8.4
---
> matplotlib==3.9.0
I'll have to check why.
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.
So I think the reason it failed was because of this line in make_deps_diff.sh
pip freeze | grep -v "evalml.git" | grep -E "${allow_list}" > "${DEPENDENCY_FILE_PATH}"
This line compares the requirements gotten from pip freeze | grep -v "evalml.git"
to the ones in allow_list
(which was made earlier in the file).
allow_list
lists nlp-primitives
while pip freeze | grep -v "evalml.git"
lists nlp_primitives
. This difference makes it so that nothing even shows up in the diff since it's looking for nlp_primitives
rather than nip-primitives
.
I changed the line to:
(pip freeze | grep -v "evalml.git") | tr "_" "-" | grep -E "${allow_list}" > "${DEPENDENCY_FILE_PATH}"
which just converts the "_" to "-".
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 once Becca's comments are resolved
Pull Request Description
Featuretools and woodwork (and whole lot of other libraries) are dropping support for python 3.8, which will (and is) causing a lot of test run errors.
Closes #4416
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.