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

Fix accidental requirement of Pandas 1.5. Bump minimum Pandas version to 0.25. Run tests with it #3130

Merged
merged 20 commits into from
Aug 1, 2023

Conversation

binste
Copy link
Contributor

@binste binste commented Jul 31, 2023

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.

@binste binste changed the title Add test with lowest supported Pandas version Run tests with lowest supported Pandas version Jul 31, 2023
@jonmmease
Copy link
Contributor

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 typing.TYPE_CHECKING, although maybe the mypy tests would fail with the older version of pandas in this case.

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.

@jonmmease
Copy link
Contributor

huh, looks like the if condition in the GitHub actions isn't working

@jonmmease
Copy link
Contributor

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 if: conditions (or always evaluating them to true).

@binste
Copy link
Contributor Author

binste commented Jul 31, 2023

I think I had the same issue recently over in #3118. I'll give it a try.

@binste
Copy link
Contributor Author

binste commented Jul 31, 2023

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.
@jonmmease
Copy link
Contributor

I'll try that again (I think again 😄 )

@jonmmease
Copy link
Contributor

Oh, looks like I needed single quotes. The issue now is with installing pandas 0.18.0 in Python 3.8:

Run pip install pandas==0.18
Collecting pandas==0.18
  Downloading pandas-0.18.0.tar.gz (7.1 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 7.1/7.1 MB 24.2 MB/s eta 0:00:00
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Getting requirements to build wheel: started
  Getting requirements to build wheel: finished with status 'done'
  Installing backend dependencies: started
  Installing backend dependencies: finished with status 'done'
  Preparing metadata (pyproject.toml): started
  Preparing metadata (pyproject.toml): finished with status 'error'
  error: subprocess-exited-with-error
  
  × Preparing metadata (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [4 lines of output]
      <string>:39: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
      error in pandas setup command: 'install_requires' must be a string or list of strings containing valid project/version requirement specifiers; Expected end or semicolon (after version specifier)
          pytz >= 20[11](https://github.com/altair-viz/altair/actions/runs/5716874306/job/15489393829?pr=3130#step:6:12)k
               ~~~~~~~^
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

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.

@jonmmease
Copy link
Contributor

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.

@binste
Copy link
Contributor Author

binste commented Jul 31, 2023

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.

@jonmmease
Copy link
Contributor

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!

@jonmmease
Copy link
Contributor

I think this is ready now @binste, thanks for raising the issues!

@binste binste changed the title Run tests with lowest supported Pandas version Bump minimum Pandas version to 0.25. Run tests with it Jul 31, 2023
@binste binste changed the title Bump minimum Pandas version to 0.25. Run tests with it Fix accidental requirement of Pandas 1.5. Bump minimum Pandas version to 0.25. Run tests with it Jul 31, 2023
@binste
Copy link
Contributor Author

binste commented Aug 1, 2023

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!

Copy link
Contributor

@jonmmease jonmmease left a 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)


Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@jonmmease jonmmease merged commit bd89d2f into vega:main Aug 1, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants