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

Add poetry and fix problem from #23 #25

Merged
merged 44 commits into from
Jun 15, 2022
Merged

Add poetry and fix problem from #23 #25

merged 44 commits into from
Jun 15, 2022

Conversation

breno-jesus-fernandes
Copy link
Contributor

Based on feedback from #23 I remove "black" and "isort" style applied. Add links to documentation in README.md. Fix version compatibility from project, python, and pandas. I merge the last main in the current branch, so should be ok this time.

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.

This all looks good, but we need to keep the tests that build and install the wheel.

.github/workflows/test.yml Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 8, 2022

I'm approving so the CI will run here. I still need to pull down this branch and test locally to get a sense of how this works.

setup.cfg Show resolved Hide resolved
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 8, 2022

I'm comparing what we have in the pandas wheel with what poetry produces.

In the wheel file produced by poetry, a file called entry_points.txt in pandas_stubs-1.4.2.20220608.dist-info is included and it has the contents:

[console_scripts]
all_tests=scripts.tests:run_all
install_wheel=scripts.tests:install_wheel
rm_src_code=scripts.tests:remove_src_code

This shouldn't appear in our wheel file as it refers to the poetry scripts. Can you figure out how to not include it?

Also, for the version number, the convention I'd like to use is YYMMDD, so can you change that as well?

This is really close to me merging it in!

…rom setuptools to "poe" a poetry plugin to run util scripts.
@breno-jesus-fernandes
Copy link
Contributor Author

I did not know that poetry.scripts it was equivalent to console-scripts from setup tools,looks like there is not way to remove from entry points, so now i'm using a simple poetry plugin to run all the util scripts called poe.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 9, 2022

I pulled your latest version, and did a poetry install -vvv to update the installation, and got this message:

 Because pandas-stubs depends on poethepoet (^0.13.1) which doesn't match any versions, version solving failed.

Seems like pypi only has version 0.0.3 .

@breno-jesus-fernandes
Copy link
Contributor Author

I think you already had a pre builded poetry.lock so came a conflicted, just type poetry update, to see if it works, all tests passed here.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 9, 2022

OK, now that works, but there are still some issues:

  1. After I do poetry run poe all_tests, the pandas-stubs package is still installed in my environment.
  2. There are now 5 tests that are run:
    • local mypy
    • local pytest
    • local pyright
    • pyright against installed wheel
    • mypy against installed wheel
      It would be useful to have some blank lines between the output of each of these tests, and markers between the outputs of the tests that say things like "Beginning mypy test on repo", "End mypy test on repo", with blank lines after the "End" messages.
  3. Can we make the order of the tests mypy, then pyright, then pytest
  4. Can we set things up to run only a subset of the tests (via a command line parameter)? i.e,. run only the local, or run only the wheel based test. And an option to run only mypy and pyright locally, skipping the pytest test. The reason I say this is that when you're working on the stubs, you often just need to run the typing tests to check for validity, as opposed to running everything.
  5. When running the tests, this message appears multiple times:
<my-path-to-environment>\lib\site-packages\setuptools\command\install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
  warnings.warn(

Can that be avoided, or is this a known issue with poetry?

@breno-jesus-fernandes
Copy link
Contributor Author

  1. When we install the project dependencies with poetry install -vvv, actually is installing the project in "editable mode" but not building the wheel and installing, here is the log:
- Building package pandas-stubs in editable mode
- Adding pandas_stubs.pth to C:\Users\Ghost\Documents\pandas-stubs\.venv\Lib\site-packages for C:\Users\Ghost\Documents\pandas-stubs
- Adding the pandas_stubs-1.4.2.220608.dist-info directory to C:\Users\Ghost\Documents\pandas-stubs\.venv\Lib\site-packages

I think is a way that poetry defines a fixed path to the project in "pandas_stubs.pth" file to know where is the source code. We could easily avoid just typing poetry install -vvv --no-root or poetry update -vvv but seems mypy and pyright stop working properly. So for now, I think we do not need to worry about this because does not seems to cause any damage to the project.

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.

Aside from the comment about reinstalling poetry, this is looking good.

When the install dist step is done, what's puzzling is the output where it seems to be uninstalling lots of things, then does the install of the dependent packages. I'm seeing this in the output:

===========================================
Beginning to run: 'Install Dist'
===========================================

Processing c:\code\pr25\dist\pandas_stubs-1.4.2.220608-py3-none-any.whl
Collecting matplotlib>=3.3.2
  Using cached matplotlib-3.5.2-cp39-cp39-win_amd64.whl (7.2 MB)
Collecting pandas<1.5.0,>=1.4.0
  Using cached pandas-1.4.2-cp39-cp39-win_amd64.whl (10.5 MB)
Collecting typing-extensions>=4.2.0
  Using cached typing_extensions-4.2.0-py3-none-any.whl (24 kB)
Collecting numpy>=1.17
  Using cached numpy-1.22.4-cp39-cp39-win_amd64.whl (14.7 MB)
Collecting fonttools>=4.22.0
  Using cached fonttools-4.33.3-py3-none-any.whl (930 kB)
Collecting python-dateutil>=2.7
  Using cached python_dateutil-2.8.2-py2.py3-none-any.whl (247 kB)
Collecting pillow>=6.2.0
  Using cached Pillow-9.1.1-cp39-cp39-win_amd64.whl (3.3 MB)
Collecting cycler>=0.10
  Using cached cycler-0.11.0-py3-none-any.whl (6.4 kB)
Collecting pyparsing>=2.2.1
  Using cached pyparsing-3.0.9-py3-none-any.whl (98 kB)
Collecting kiwisolver>=1.0.1
  Using cached kiwisolver-1.4.2-cp39-cp39-win_amd64.whl (55 kB)
Collecting packaging>=20.0
  Using cached packaging-21.3-py3-none-any.whl (40 kB)
Collecting pytz>=2020.1
  Using cached pytz-2022.1-py2.py3-none-any.whl (503 kB)
Collecting six>=1.5
  Using cached six-1.16.0-py2.py3-none-any.whl (11 kB)
Installing collected packages: six, pyparsing, pytz, python-dateutil, pillow, packaging, numpy, kiwisolver, fonttools, cycler, typing-extensions, pandas, matplotlib, pandas-stubs
  Attempting uninstall: six
    Found existing installation: six 1.16.0
    Uninstalling six-1.16.0:
      Successfully uninstalled six-1.16.0
  Attempting uninstall: pyparsing
    Found existing installation: pyparsing 3.0.9
    Uninstalling pyparsing-3.0.9:
      Successfully uninstalled pyparsing-3.0.9
  Attempting uninstall: pytz
    Found existing installation: pytz 2022.1
    Uninstalling pytz-2022.1:
      Successfully uninstalled pytz-2022.1
  Attempting uninstall: python-dateutil
    Found existing installation: python-dateutil 2.8.2
    Uninstalling python-dateutil-2.8.2:
      Successfully uninstalled python-dateutil-2.8.2
  Attempting uninstall: pillow
    Found existing installation: Pillow 9.1.1
    Uninstalling Pillow-9.1.1:
      Successfully uninstalled Pillow-9.1.1
  Attempting uninstall: packaging
    Found existing installation: packaging 21.3
    Uninstalling packaging-21.3:
      Successfully uninstalled packaging-21.3
  Attempting uninstall: numpy
    Found existing installation: numpy 1.22.4
    Uninstalling numpy-1.22.4:
      Successfully uninstalled numpy-1.22.4
  Attempting uninstall: kiwisolver
    Found existing installation: kiwisolver 1.4.2
    Uninstalling kiwisolver-1.4.2:
      Successfully uninstalled kiwisolver-1.4.2
  Attempting uninstall: fonttools
    Found existing installation: fonttools 4.33.3
    Uninstalling fonttools-4.33.3:
      Successfully uninstalled fonttools-4.33.3
  Attempting uninstall: cycler
    Found existing installation: cycler 0.11.0
    Uninstalling cycler-0.11.0:
      Successfully uninstalled cycler-0.11.0
  Attempting uninstall: typing-extensions
    Found existing installation: typing-extensions 4.2.0
    Uninstalling typing-extensions-4.2.0:
      Successfully uninstalled typing-extensions-4.2.0
  Attempting uninstall: pandas
    Found existing installation: pandas 1.4.2
    Uninstalling pandas-1.4.2:
      Successfully uninstalled pandas-1.4.2
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.5.2
    Uninstalling matplotlib-3.5.2:
      Successfully uninstalled matplotlib-3.5.2
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
poetry 1.1.7 requires keyring<22.0.0,>=21.2.0; python_version >= "3.6" and python_version < "4.0", but you have keyring 23.4.0 which is incompatible.
poetry 1.1.7 requires packaging<21.0,>=20.4, but you have packaging 21.3 which is incompatible.
Successfully installed cycler-0.11.0 fonttools-4.33.3 kiwisolver-1.4.2 matplotlib-3.5.2 numpy-1.22.4 packaging-21.3 pandas-1.4.2 pandas-stubs-1.4.2.220608 pillow-9.1.1 pyparsing-3.0.9 python-dateutil-2.8.2 pytz-2022.1 six-1.16.0 typing-extensions-4.2.0

===========================================
End 'Install Dist', runtime: 28.683 seconds.
===========================================

Shouldn't installing the distribution just have to install the built wheel and not reinstall everything? This seems to be adding a lot of extra time.

Maybe it is the --force_reinstall that is doing that ?

scripts/test.py Outdated
Step(name="Run Pyright Against Dist", run=run_pyright_dist),
Step(name="Uninstall Dist", run=uninstall_dist),
Step(name="Restore Source Code", run=restore_src),
Step(name="Install Poetry", run=install_poetry),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you need to install poetry when it is already installed?

scripts/test.py Outdated
Step(name="Run Pyright Against Dist", run=run_pyright_dist),
Step(name="Uninstall Dist", run=uninstall_dist),
Step(name="Restore Source Code", run=restore_src),
Step(name="Install Poetry", run=install_poetry),
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above about installing poetry

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 12, 2022

I think there is one more issue to check. Suppose that there is an error that occurs in mypy or pyright when testing the distribution via poetry run poe test_dist . Then the test will exit, and the source will not be restored.

To see this, add this line to tests/test_timefuncs.py:

 def test_types_init() -> None:
      ts: pd.Timedelta = pd.Timestamp("2021-03-01T12")

Then do poetry run poe test_dist .

So is it possible that if an error is discovered when testing the distribution that you automatically restore the source?

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 12, 2022

Also, if there is an error when testing the distribution, you should uninstall the distribution.

@breno-jesus-fernandes
Copy link
Contributor Author

Not sure if I added in this PR, but I have finished the pipeline to publish in github and pypi the package through github actions.

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.

This is good to go. Thanks @BrenoJesusFernandes .

@Dr-Irv Dr-Irv merged commit f2b783c into pandas-dev:main Jun 15, 2022
@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Jun 15, 2022

Not sure if I added in this PR, but I have finished the pipeline to publish in github and pypi the package through github actions.

Please submit that as a separate PR.

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