-
Notifications
You must be signed in to change notification settings - Fork 795
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
Fix accidental requirement of Pandas 1.5. Bump minimum Pandas version to 0.25. Run tests with it #3130
Conversation
Ahh, thanks for flagging this @binste, and for working on testing with older pandas. I'll look into alternatives for the type signature, it might be enough to wrap the import in In terms of pandas versions, I personally think we could bump the pandas requirement up to 1.0 (released 3.5 years ago) in an Altair minor version, but I agree that 1.5 (released less than 1 year ago) would be too aggressive. |
huh, looks like the |
70544aa
to
b1e07f2
Compare
I'm really stumped as to why GitHub actions is always evaluating the "Maybe uninstall optional dependencies" and "Maybe install lowest supported Pandas version" jobs, ignoring the |
I think I had the same issue recently over in #3118. I'll give it a try. |
Seems like you're trying it already. In #3118 I got it working for jsonschema with this line https://github.com/altair-viz/altair/pull/3118/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R32 |
This reverts commit a612f9a.
I'll try that again (I think again 😄 ) |
Oh, looks like I needed single quotes. The issue now is with installing pandas 0.18.0 in Python 3.8:
Looks like pandas 0.18.1 didn't ship wheels for Python 3.8. I'll check what the oldest version of pandas is that includes wheels for 3.8 is. |
The oldest version of pandas with 3.8 wheels is 0.25.3, released 2019-11-01. I'm personally inclined to update our min supported pandas version to this one for Altair 5.1 so that we don't need to build pandas wheels from source on CI. |
Thanks for figuring out to use single quotes, weird... Bumping to minimum version 0.25.3 sounds very reasonable to me too. We can rename this PR and do it all in here. I think there is some code logic somewhere which is for Pandas 0.18. I'll try to find and remove that one too while we're at it. I can offer to do the Pandas bump and cleanup later this week if you prefer to focus on the issue with the type hint. |
Sounds good. I'll take this PR and update the min pandas and numpy versions and fix the type hint issue (I'll need to update these versions to get CI working). I won't look for any pandas/numpy compatibility code that could be removed, so if you could do a pass on that at some point that would be great! |
I think this is ready now @binste, thanks for raising the issues! |
fce0b25
to
f8d98ca
Compare
Great, I removed some part of the code which is now redundant and clarified another one which we need to keep but for other reasons compared to why it was originally implemented. @jonmmease Could you do a review of my last commits? Feel free to merge afterwards. 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.
Looks good. Thanks!
else: | ||
return _infer_dtype(value) | ||
|
||
|
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.
🎉
This import introduced in #3114 is not available in Pandas < 1.5 and Altair currently supports Pandas >= 0.18. This PR adds a test run with the lowest supported Pandas version.
@jonmmease Do you know of a good way to make that type check independent of the pandas import? Else, we could fall back on not type-hinting it if necessary. Feel free to push directly to this branch if you have an idea and want to try it out in the GitHub actions workflow.
Pandas 0.18 is from 2016 and we could for sure consider bumping it but I don't think we can go to Pandas 1.5 just yet. Especially not in a minor release of Altair.