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

(🎁) Support Python 3.11 #398

Closed
wants to merge 2 commits into from
Closed

Conversation

KotlinIsland
Copy link

@KotlinIsland KotlinIsland commented Oct 25, 2022

  • Closes #xxxx (Replace xxxx with the Github issue number)
  • Tests added: Please use assert_type() to assert the type of any return value

@KotlinIsland KotlinIsland changed the title Support Python 3.11 (🎁) Support Python 3.11 Oct 25, 2022
@bashtage
Copy link
Contributor

Need to wait for lxml to have 3.11 wheels.

@KotlinIsland
Copy link
Author

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 25, 2022

I think this won't build because there are no wheels for 3.11 for a lot of the dev dependencies, but it should be valid to publish I think.

pandas doesn't support version 3.11 so I don't think we should be running tests or saying that pandas-stubs supports 3.11 either.

I'm open to being convinced otherwise.

@bashtage
Copy link
Contributor

There are 3.11 wheels on PyPu if 1.5.1, so it looks like 3.11 is supported by pandas.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 25, 2022

There are 3.11 wheels on PyPu if 1.5.1, so it looks like 3.11 is supported by pandas.

Fair point. We didn't document it, so I created an issue here to fix that: pandas-dev/pandas#49301

CI is failing due to lack of 3.11 version for arrow

@breno-jesus-fernandes
Copy link
Contributor

Be careful, looks like we still don't have a stable version for python 3.11 for macos in github actions: https://github.com/actions/python-versions/releases/tag/3.11.0-3320066239

@Dr-Irv Dr-Irv mentioned this pull request Oct 26, 2022
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 26, 2022

CI is failing because no stable version of python 3.11 for MacOS yet and no pyarrow for python 3.11

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 26, 2022

Issue is still that pyarrow doesn't work with python 3.11. The CI sees no binary, then tries to build the wheel, and it fails.

There might be a workaround here to skip the requirement of pyarrow when testing on 3.11 (and then to adjust the tests accordingly to do a pytest.skip) but I don't know how to modify pyproject.toml to do that.

@BrenoJesusFernandes Any ideas on how to make the poetry requirement for pyarrow conditional on python <= 3.10 in pyproject.toml ?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 26, 2022

spoke with someone who is familiar with pyarrow, and they think 3.11 wheels will be available within a week, so maybe easiest to wait for that

@breno-jesus-fernandes
Copy link
Contributor

I don't get why pandas is available in python 3.11, when one requirement is not available. It's seems wrong for me. In poetry last versio, you can define multiples dependencies besides the dev-dependecies, but I think we should wait the dependecies to be available in 3.11.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 26, 2022

I don't get why pandas is available in python 3.11, when one requirement is not available. It's seems wrong for me. In poetry last versio, you can define multiples dependencies besides the dev-dependecies, but I think we should wait the dependecies to be available in 3.11.

pyarrow is an optional dependency. So that's why pandas is available for python 3.11, because you can use pandas with python 3.11, you just can't use pyarrow with it.

So if there is a way to define the dependencies as one set for python <= 3.10 that we have now, and another set that is the same, except removes pyarrow for python 3.11, we can move forward. Then we modify the tests that use pyarrow to test if pyarrow is available. That's how the tests for pandas with python 3.11 are set up.

Or we wait a week (hopefully) until pyarrow builds wheels for python 3.11

@breno-jesus-fernandes
Copy link
Contributor

is there some test using pyarrow? if exits tests like that it is gonna fail in CI.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 26, 2022

is there some test using pyarrow? if exits tests like that it is gonna fail in CI.

There are a couple of tests in test_pandas.py and test_frame.py. They are protected with a pytest.importorskip .

So we just need to modify the pyproject.toml file to indicate that pyarrow should not be installed when testing python 3.11. Then the tests should work.

@breno-jesus-fernandes
Copy link
Contributor

hmm I see, so it's simple than I thought, just add something like that:

[tool.poetry.dev-dependencies]
pyarrow = { version = "currently-version", python = "<3.11" }
]

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 26, 2022

@KotlinIsland if you want to try what @BrenoJesusFernandes suggests above, that might make the CI work.

@bellini666
Copy link

Since this is a stubs package, does it make sense to limit the upper bound of the python version? I would say that even libs shouldn't do that themselves, unless there's a very good reason.

This is very limiting for people using poetry for example due to this issue

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Oct 27, 2022

Since this is a stubs package, does it make sense to limit the upper bound of the python version? I would say that even libs shouldn't do that themselves, unless there's a very good reason.

The limit is there for now because we can't run a successful test with python 3.11. We're not willing to release the stubs for a python version unless we can successfully test the stubs on that version.

In the past, we have had issues with the stubs being compatible with one version of python, but not another. So our tests help make sure we are not making that mistake.

@KotlinIsland
Copy link
Author

Looks like tables and lxml don't have 3.11 wheels either.

@KotlinIsland
Copy link
Author

KotlinIsland commented Oct 30, 2022

@Dr-Irv Okay, I have made arrow, tables and lxml conditional dependencies, and I have added a skip check for 3.11

We aren't using these dependencies directly, so I'm not quite sure why they are even specified in the dependencies, perhaps as a constraint? If not I think they should be removed.

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

So the tests revealed another issue, which is that we'd have to use mypy 0.982 to get python 3.11 support. But we have to wait for mypy to fix an issue that causes mypy to not accept our stubs. See #336 and the discussion therein.

Based on this comment: python/mypy#13781 (comment) we have to wait for version 0.990, which hopefully will be released soon.

tests/test_io.py Outdated
@@ -69,6 +70,10 @@
CWD = os.path.split(os.path.abspath(__file__))[0]


if sys.version_info >= (3, 11):
# This is only needed temporarily due to no wheels being available for arrow on 3.11
_arrow = pytest.importorskip("arrow")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have any arrow tests in test_io.py, so I don't think this is necessary here.

Copy link
Author

@KotlinIsland KotlinIsland Nov 1, 2022

Choose a reason for hiding this comment

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

Strange, it made the tests pass. Hmm perhaps for the wrong reason

Copy link
Author

@KotlinIsland KotlinIsland Nov 1, 2022

Choose a reason for hiding this comment

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

FAILED tests/test_io.py::test_orc - ImportError: Missing optional dependency 'pyarrow'.  Use pip or conda to install pyarrow.
FAILED tests/test_io.py::test_orc_path - ImportError: Missing optional dependency 'pyarrow'.  Use pip or conda to install pyarrow.
FAILED tests/test_io.py::test_orc_buffer - ImportError: Missing optional dependency 'pyarrow'.  Use pip or conda to install pyarrow.
FAILED tests/test_io.py::test_orc_columns - ImportError: Missing optional dependency 'pyarrow'.  Use pip or conda to install pyarrow.
FAILED tests/test_io.py::test_orc_bytes - ImportError: Missing optional dependency 'pyarrow'.  Use pip or conda to install pyarrow.
FAILED tests/test_io.py::test_xml - ImportError: lxml not found, please install or use the etree parser.
FAILED tests/test_io.py::test_xml_str - ImportError: lxml not found, please install or use the etree parser.
FAILED tests/test_io.py::test_hdf - ImportError: Missing optional dependency 'pytables'.  Use pip or conda to install pytables.
FAILED tests/test_io.py::test_hdfstore - ImportError: Missing optional dependency 'pytables'.  Use pip or conda to install pytables.
FAILED tests/test_io.py::test_read_hdf_iterator - ImportError: Missing optional dependency 'pytables'.  Use pip or conda to install pytables.
FAILED tests/test_io.py::test_hdf_context_manager - ImportError: Missing optional dependency 'pytables'.  Use pip or conda to install pytables.
FAILED tests/test_io.py::test_hdf_series - ImportError: Missing optional dependency 'pytables'.  Use pip or conda to install pytables.
FAILED tests/test_io.py::test_parquet - ImportError: Unable to find a usable engine; tried using: 'pyarrow', 'fastparquet'.
FAILED tests/test_io.py::test_parquet_options - ImportError: Unable to find a usable engine; tried using: 'pyarrow', 'fastparquet'.
FAILED tests/test_io.py::test_feather - ImportError: Missing optional dependency 'pyarrow'.  Use pip or conda to install pyarrow.
FAILED tests/test_io.py::test_read_html - ImportError: lxml not found, please install it

This is the output of pytest, it seems to me as if test_io is in some way using pyarrow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the output of pytest, it seems to me as if test_io is in some way using pyarrow?

I'm guessing there are dependencies. So test_orc pattern needs pyarrow, test_xml pattern needs lxml, test_hdf pattern needs pytables, etc. So we should do the skips based on the particular test (and underlying library) being done.

Copy link
Author

Choose a reason for hiding this comment

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

Hows that look?

@KotlinIsland
Copy link
Author

KotlinIsland commented Nov 2, 2022

Looks like there is some issues with mypy: python/mypy#13627

This is causing the ci to fail for 3.11 and will require updating mypy to 0.982

I've created a separate PR to handle this here #406

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 2, 2022

@KotlinIsland Please see #398 (review)

We have to wait for mypy 0.990

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 8, 2022

@KotlinIsland Please see #398 (review)

We have to wait for mypy 0.990

@KotlinIsland We are now testing with mypy 0.990 via #419 so can you merge with that, resolve conflicts, and see if the python 3.11 support will work?

@KotlinIsland
Copy link
Author

@Dr-Irv Can you trigger the CI?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 10, 2022

Currently two issues (but there might be more):
In test_pandas.py, have to use a pattern like this:

import pandas.util._test_decorators as td  # at top of file somewhere

pyarrow_skip = td.skip_if_no("pyarrow")


@pyarrow_skip
def test_arrow_dtype() -> None:
    import pyarrow as pa

    check( ...

and in pandas-stubs/core/arrays/arrow/dtype.pyi (somewhere near the top):

if sys.version_info < (3, 11):
    import pyarrow as pa
else:
    pa = Any

You may also need to put double quotes around each reference to pa.DataType in that file.

I'd also suggest setting up a local environment with python 3.11 and running the tests locally instead of waiting for me or someone else to do a CI approval.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 11, 2022

I played around a bit with this, and uncovered a pyright bug that doesn't ignore imports based on the python version test. microsoft/pyright#4182

So we have to wait for that to get fixed to be able to do conditional imports.

)

pyarrow_skip = pytest.mark.skip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be

pyarrow_skip = pytest.mark.skipif(sys.version_info >= (3, 11), "pyarrow is not available for 3.11 yet")
"""This is only needed temporarily due to no wheels being available for arrow on 3.11"""

from tests import (
TYPE_CHECKING_INVALID_USAGE,
check,
importskip
Copy link
Collaborator

Choose a reason for hiding this comment

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

this import is failing.

Copy link
Contributor

@bashtage bashtage left a comment

Choose a reason for hiding this comment

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

Some fixes needed.

arrow_skip = pytest.mark.skipif(sys.version_info >= (3, 11), "pyarrow is not available for 3.11 yet")
"""This is only needed temporarily due to no wheels being available for arrow on 3.11"""

lxml_skip = pytest.mark.skipif(sys.version_info >= (3, 11), "pyarrow is not available for 3.11 yet")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong reason in the explanation here. Should be lxml not arrow.

def test_arrow_dtype() -> None:
pytest.importorskip("pyarrow")
Copy link
Contributor

Choose a reason for hiding this comment

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

These is there for a reason to protect Windows. I think it should stay.

)

pyarrow_skip = pytest.mark.skip
Copy link
Contributor

Choose a reason for hiding this comment

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

Appears to be not needed.

@Dr-Irv Dr-Irv mentioned this pull request Nov 20, 2022
1 task
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 20, 2022

I created #435 to move this along. Took some of your ideas from here. Had to handle an issue with pandas that's a bit thorny and want to see if I can coordinate release of this with release of pandas 1.5.2 if a PR there is included.

Thanks for working on this. It was a great head start.

@Dr-Irv Dr-Irv closed this Nov 20, 2022
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.

5 participants