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 signature of Series.map() #942

Merged
merged 2 commits into from
Jun 27, 2024
Merged

Conversation

JanEricNitschke
Copy link
Contributor

@JanEricNitschke JanEricNitschke commented Jun 24, 2024

Only added the proposed changes so far. Will try to work on tests as soon as possible.

Does not seem to break any existing tests so far at least.

(I think the failures are all due to numpy 2.0 and separate from my changes)

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 24, 2024

(I think the failures are all due to numpy 2.0 and separate from my changes)

You could fix that by doing this in pyproject.toml :

numpy = [
  { version = ">=1.23.5, <2.0.0", python = ">=3.9,<3.12" },
  { version = ">=1.26.0, <2.0.0", python = ">=3.12,<3.13" }
]

We haven't tested the stubs yet with numpy 2.0, and I think it might be a bit of work for us to get that to work with both the old and new numpy versions.

@JanEricNitschke
Copy link
Contributor Author

JanEricNitschke commented Jun 25, 2024

I have now added two tests for this that fail without the changes and pass with them.

I have done the pyproject.toml changes locally for this. There was still one test failure for test_show_version.

I wouldnt put the pyproject.toml into this PR because i feel i wouldnt want to restrict my runtime dependencies just for the stubs, especially since they may still correctly type hint my use case (which for me personally they do as far as i know)

@JanEricNitschke JanEricNitschke marked this pull request as ready for review June 25, 2024 16:02
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 25, 2024

So the CI is failing because it installed numpy 2.0. I think I have a fix for that. I am going to try that out and if it works, get that merged in, then you can merge in that change, and I think your CI will then succeed.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 26, 2024

@JanEricNitschke we just merged in the changes so the CI will work with numpy 2.0. Can you fetch main, merge it into your branch, and push?

@JanEricNitschke
Copy link
Contributor Author

JanEricNitschke commented Jun 27, 2024

Rebased. Locally i still have the test_show_version test fail (regardless of my changes). Not sure why that is but lets see if that also happens here.

Also: Do you accept PRs for things that only affect pyright strict mode? In that case the added tests might not fail without the changes with the current pyright settings of pandas-stubs. (Dont have such an example yet, but might in the future as i am trying to get a project that uses pandas to type check in strict mode)

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 27, 2024

Rebased. Locally i still have the test_show_version test fail (regardless of my changes). Not sure why that is but lets see if that also happens here.

We know that test is failing because of tables 3.9.2 not being updated to being compatible with numpy 2.0. There is an issue in the tables repo about that, and we also have an issue in pandas to make sure that show_versions() works properly when pytables import is failing.

So I believe if you removed tables from your python environment, that test will pass, although I thought I had instrumented things to make that test_show_versions() test pass with numpy 2.0 and tables installed. In fact, that seems to be the case. The CI on your PR succeeded, and the python 3.12 tests are using numpy 2.0 now.

There's a possibility that show_versions() is failing due to a different package not being compatible with numpy 2.0. Can you just do a pd.show_versions() in the interpreter and share the output? If it is not about tables, then you can add information to this pandas issue: pandas-dev/pandas#59110

Also: Do you accept PRs for things that only affect pyright strict mode? In that case the added tests might not fail without the changes with the current pyright settings of pandas-stubs. (Dont have such an example yet, but might in the future as i am trying to get a project that uses pandas to type check in strict mode)

As long as any fix that is good for strict mode works with the current settings we use for pyright, we will accept it. I realize there's a possibility that we wouldn't be testing that, but then we'd have to add a CI job to do tests of pyright with strict mode, which may be non-trivial to do.

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.

@Dr-Irv Dr-Irv merged commit 54a763c into pandas-dev:main Jun 27, 2024
13 checks passed
@JanEricNitschke
Copy link
Contributor Author

@Dr-Irv

The output of show_versions is:

INSTALLED VERSIONS
------------------
commit                : d9cdd2ee5a58015ef6f4d15c7226110c9aab8140
python                : 3.11.3.final.0
python-bits           : 64
OS                    : Windows
OS-release            : 10
Version               : 10.0.19045
machine               : AMD64
processor             : AMD64 Family 25 Model 33 Stepping 0, AuthenticAMD
byteorder             : little
LC_ALL                : None
LANG                  : en_US.UTF-8
LOCALE                : de_DE.cp1252

pandas                : 2.2.2
numpy                 : 1.26.4
pytz                  : 2024.1
dateutil              : 2.9.0.post0
setuptools            : None
pip                   : 23.1.2
Cython                : None
pytest                : 8.2.2
hypothesis            : None
sphinx                : None
blosc                 : None
feather               : None
xlsxwriter            : 3.2.0
lxml.etree            : 5.2.2
html5lib              : 1.1
pymysql               : None
psycopg2              : None
jinja2                : 3.1.4
IPython               : None
pandas_datareader     : None
adbc-driver-postgresql: None
adbc-driver-sqlite    : None
bs4                   : 4.12.3
bottleneck            : None
dataframe-api-compat  : None
fastparquet           : None
fsspec                : None
gcsfs                 : None
matplotlib            : 3.8.4
numba                 : None
numexpr               : 2.10.1
odfpy                 : None
openpyxl              : 3.1.4
pandas_gbq            : None
pyarrow               : 16.1.0
pyreadstat            : 1.2.7
python-calamine       : None
pyxlsb                : 1.0.10
s3fs                  : None
scipy                 : 1.13.1
sqlalchemy            : 2.0.31
tables                : 3.9.2
tabulate              : 0.9.0
xarray                : 2024.6.0
xlrd                  : 2.0.1
zstandard             : None
tzdata                : 2024.1
qtpy                  : None
pyqt5                 : None

The locally failing test gives:

    def test_show_version():
>       with pytest_warns_bounded(
            UserWarning,
            match="Setuptools is replacing distutils",
            upper="3.11.99",
            version_str=platform.python_version(),
        ):
E       Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E        Emitted warnings: [].

tests\test_utility.py:19: Failed
---------------------------------------------------------------------------------------------------------------------------------------- Captured stdout call ----------------------------------------------------------------------------------------------------------------------------------------- 
{
  "system": {
    "commit": "d9cdd2ee5a58015ef6f4d15c7226110c9aab8140",
    "python": "3.11.3.final.0",
    "python-bits": 64,
    "OS": "Windows",
    "OS-release": "10",
    "Version": "10.0.19045",
    "machine": "AMD64",
    "processor": "AMD64 Family 25 Model 33 Stepping 0, AuthenticAMD",
    "byteorder": "little",
    "LC_ALL": null,
    "LANG": "en_US.UTF-8",
    "LOCALE": {
      "language-code": "de_DE",
      "encoding": "cp1252"
    }
  },
  "dependencies": {
    "pandas": "2.2.2",
    "numpy": "1.26.4",
    "pytz": "2024.1",
    "dateutil": "2.9.0.post0",
    "setuptools": null,
    "pip": "23.1.2",
    "Cython": null,
    "pytest": "8.2.2",
    "hypothesis": null,
    "sphinx": null,
    "blosc": null,
    "feather": null,
    "xlsxwriter": "3.2.0",
    "lxml.etree": "5.2.2",
    "html5lib": "1.1",
    "pymysql": null,
    "psycopg2": null,
    "jinja2": "3.1.4",
    "IPython": null,
    "pandas_datareader": null,
    "adbc-driver-postgresql": null,
    "adbc-driver-sqlite": null,
    "bs4": "4.12.3",
    "bottleneck": null,
    "dataframe-api-compat": null,
    "fastparquet": null,
    "fsspec": null,
    "gcsfs": null,
    "matplotlib": "3.8.4",
    "numba": null,
    "numexpr": "2.10.1",
    "odfpy": null,
    "openpyxl": "3.1.4",
    "pandas_gbq": null,
    "pyarrow": "16.1.0",
    "pyreadstat": "1.2.7",
    "python-calamine": null,
    "pyxlsb": "1.0.10",
    "s3fs": null,
    "scipy": "1.13.1",
    "sqlalchemy": "2.0.31",
    "tables": "3.9.2",
    "tabulate": "0.9.0",
    "xarray": "2024.6.0",
    "xlrd": "2.0.1",
    "zstandard": null,
    "tzdata": "2024.1",
    "qtpy": null,
    "pyqt5": null
  }
}
INSTALLED VERSIONS
------------------
commit                : d9cdd2ee5a58015ef6f4d15c7226110c9aab8140
python                : 3.11.3.final.0
python-bits           : 64
OS                    : Windows
OS-release            : 10
Version               : 10.0.19045
machine               : AMD64
processor             : AMD64 Family 25 Model 33 Stepping 0, AuthenticAMD
byteorder             : little
LC_ALL                : None
LANG                  : en_US.UTF-8
LOCALE                : de_DE.cp1252

pandas                : 2.2.2
numpy                 : 1.26.4
pytz                  : 2024.1
dateutil              : 2.9.0.post0
setuptools            : None
pip                   : 23.1.2
Cython                : None
pytest                : 8.2.2
hypothesis            : None
sphinx                : None
blosc                 : None
feather               : None
xlsxwriter            : 3.2.0
lxml.etree            : 5.2.2
html5lib              : 1.1
pymysql               : None
psycopg2              : None
jinja2                : 3.1.4
IPython               : None
pandas_datareader     : None
adbc-driver-postgresql: None
adbc-driver-sqlite    : None
bs4                   : 4.12.3
bottleneck            : None
dataframe-api-compat  : None
fastparquet           : None
fsspec                : None
gcsfs                 : None
matplotlib            : 3.8.4
numba                 : None
numexpr               : 2.10.1
odfpy                 : None
openpyxl              : 3.1.4
pandas_gbq            : None
pyarrow               : 16.1.0
pyreadstat            : 1.2.7
python-calamine       : None
pyxlsb                : 1.0.10
s3fs                  : None
scipy                 : 1.13.1
sqlalchemy            : 2.0.31
tables                : 3.9.2
tabulate              : 0.9.0
xarray                : 2024.6.0
xlrd                  : 2.0.1
zstandard             : None
tzdata                : 2024.1
qtpy                  : None
pyqt5                 : None

Great, just wanted to make sure because it said somewhere that there should always be a test that fails without and works with. But i did realize that that would be a lot of work to setup.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 27, 2024

@JanEricNitschke I think the issue here is that you don't have setuptools installed. If you did, you would get the message.

If you had followed these instructions: https://github.com/pandas-dev/pandas-stubs/blob/main/docs/setup.md

Then I think setuptools would have been installed and then the test would have passed.

@JanEricNitschke
Copy link
Contributor Author

JanEricNitschke commented Jun 27, 2024

@Dr-Irv i actually did follow those steps. Except i had already installed poetry via pipx. I guess that might cause the difference?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 27, 2024

@JanEricNitschke Not sure. The error is definitely because setuptools is not installed, so add that to the environment by hand and see what happens. In my case, when I created the conda environment with python, setuptools was included by default.

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