-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
DEPS: Unifying testing and building dependencies across builds #29678
Conversation
@jreback Looks like all the reasons for the different versions are now gone, except the one for 32 bits (which will still use the same version, but from pip instead of conda) |
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.
you have very subtley changed things, pls restore as per comments.
ci/deps/azure-36-locale_slow.yaml
Outdated
- python=3.6.* | ||
# tools | ||
- cython=0.29.13 | ||
- pytest |
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.
need a min pin on pytest (for all .yaml)
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.
We've got all these versions in the builds:
- pytest
- pytest>=4.0.2
- pytest>=4.0.2,<5.0
- pytest=4.5.0
- pytest>=5.0.0
- pytest>=5.0.1
Any preference on which you want to require? pytest>-5.0.1
is ok?
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.
yeah ok with the >=4.0.2 I think < 4 we don't support at all
- pytest-mock | ||
- hypothesis>=3.58.0 | ||
- pytest-azurepipelines | ||
# pandas dependencies |
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.
can you put a blank line before the comments
ci/deps/azure-37-locale.yaml
Outdated
@@ -3,8 +3,16 @@ channels: | |||
- defaults | |||
- conda-forge | |||
dependencies: | |||
- python=3.7.* | |||
# tools | |||
- cython=0.29.13 |
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.
these should be >=
ci/deps/azure-37-numpydev.yaml
Outdated
- pytest>=4.0.2,<5.0 | ||
- pytest-xdist | ||
# tools | ||
- cython=0.29.13 |
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.
same (and all others except for the 3.6-min build can have >= for cython)
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.
In the minimum versions build we have a >=0.29.13
now. There is a =0.29.13
in another build, not sure if that was intentional.
I guess if we pin cython, when this causes a failure, the immediate action will be to pin it to something bigger where it works. So, is it really worth forcing in a build the 0.29.3
version?
I addressed the comments. My understanding (and I can surely be missing something) is that we shouldn't use the same rationale for the dependencies and our tools. For dependencies (e.g. numpy), we want to make sure that pandas works with 1.13.3 and above, so we test that version, the latest, and probably something in between. Users installing pandas that want a specific version can do that, as far as it's supported. But for tools (e.g. pytest), we don't care about users in terms of versions. It's something we use as developers, and users don't install or care about. For what I know, I think it's common to just decide a version (the latest unless there is a reason to use another), we all use the same, and we don't care about compatibility... Pinning a specific version would make conda solver life much easier (faster), but we'd need to manually keep increasing it if we don't want to end up with old versions. So, I guess no version or a I'd say cython is more in the tools side, but users can compile pandas themselves, so we can consider supporting more than a version. But I think it's also reasonable to say that pandas compiles with Cython version X, and not bother on supporting multiple versions for something where it probably doesn't add any value. Users may have code in numpy 1.13 and the latest pandas, and we want to let them run it. But I don't see the advantage of telling users they can compile pandas with Cython 0.28 and 0.29. They should compile in 0.29 since the generated code should be better. |
@datapythonista we DO care about the tools versions. Having tool much churn on these is bad. Other things being equal, we also want as much compatibility as possible. We like to keep things as stable as possible; when you are a developer and come back to a project, having everything break is a bad developer experience. So this is a balance, between using the later-latest tools and keeping some compatibility. We are already having a lot of churn with black/mypy moving pretty fast, so changing the world otherwise is not great. |
For cython we have very specific minimum versions, you cannot use it with earlier versions. Similar to the other tools, we try not to move this too often. |
- cython>=0.29.13 | ||
- pytest>=4.0.2 | ||
- pytest-xdist>=1.21 |
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.
actually looking at some of our builds, I think we need pytest-xdist>1.29, which likely implies pytest>5.0.1, if you can confirm this, we can move the pytest pin.
I think that we are likely installing the latest pytest which is fine; these are just a min pin. So on the 36-min versions, can you pin exactly these versions to test.
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.
in all our builds we have pytest-xdist>=1.21
, happy to force 1.29
and pytest>5.0.1
but I don't think it's needed.
I agree with your previous comments. I think we do care about the versions. My point was that I don't think we care about supporting old versions, and we should probably just use the latest. Probably not for black. It's already quite annoying to use it, and if at every new version it reformats the code, we should probably pin it and update it once per year.
For pytest I care less (using the latest on CI is good for me), but for cython I think we should have a build that pins to the oldest supported (which is often very recent anyway). |
I also don't care about the pytest version. |
@datapythonista can you rebase (as added the 3.8 build). also I would like to fix pytest on the 3.6 min build to what we say the min is (4.02 or 5.01) whatever works. cython ok with fixing that on the oldest build, on the others >= is ok. |
Thanks for the comments. I'll leave the pytest upgrade for a follow up, so we can have a discussion about the version we want first. |
- cython>=0.29.13 | ||
- pytest>=5.0.1 | ||
- pytest-xdist>=1.29.0 # The rest of the builds use >=1.21, and use pytest-mock |
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.
@TomAugspurger is there any reason for this difference, now that things are standardized?
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.
I'm not aware of a reason for having this at >=1.29
Tests actually fail with pytest 4.0.2. They should work with 4.5 (we have a build with it), but I upgraded the versions everywhere to require 5.0.1. |
thanks @datapythonista as a followup can you update docs in places where we specify pytest version (contributing guide maybe, though I think might be a few places, including show_versions()). its a develop dependency but users can also run tests with it. this may warrant a release note as well. |
In our CI builds we're using different versions of pytest, pytest-xdist and other tools. Looks like Python 3.5 was forcing part of this, since some packages were not available.
In this PR I standardize the tools and versions we use in all packages, with only two exceptions:
Cython
from pip instead of conda, since conda doesn't have the version we wantpytest-azurepipelines
is installed only in builds currently running in pipelinesI also reorganize a bit the builds so dependencies are easier to find. First is always Python, then the block with the tools, which is always the same with the exceptions mentioned above, and finally the dependencies.