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 pip 20.2 test for tox #4814

Merged
merged 34 commits into from Dec 24, 2020
Merged

add pip 20.2 test for tox #4814

merged 34 commits into from Dec 24, 2020

Conversation

dswij
Copy link
Contributor

@dswij dswij commented Sep 30, 2020

Closes #4710

Add install command for pip-20.2 to test for upcoming pip resolver

TODO:

  • Fix PyDispatcher dependency
  • Mark the test failure as expected
  • Minimize dependency version changes

@wRAR
Copy link
Member

wRAR commented Oct 1, 2020

"pyopenssl 18.0.0 depends on cryptography>=2.2.1"

@dswij
Copy link
Contributor Author

dswij commented Oct 3, 2020

Seems like some tests are failing on pypy due to the new --use-feature=2020-resolver pip.

@Gallaecio
Copy link
Member

Weird errors. Let’s re-run the tests, see if they are reproducible.

@Gallaecio Gallaecio closed this Oct 5, 2020
@Gallaecio Gallaecio reopened this Oct 5, 2020
tox.ini Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #4814 (b83a1a6) into master (d0af008) will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4814      +/-   ##
==========================================
+ Coverage   87.89%   87.99%   +0.09%     
==========================================
  Files         158      158              
  Lines        9719     9719              
  Branches     1433     1433              
==========================================
+ Hits         8543     8552       +9     
+ Misses        921      911      -10     
- Partials      255      256       +1     
Impacted Files Coverage Δ
scrapy/core/downloader/__init__.py 90.97% <0.00%> (-1.51%) ⬇️
scrapy/core/scraper.py 89.57% <0.00%> (+2.45%) ⬆️
scrapy/utils/ssl.py 70.73% <0.00%> (+17.07%) ⬆️

@dswij
Copy link
Contributor Author

dswij commented Oct 6, 2020

So after having time to look at it further, there are some quirks on using the new pip resolver that failed some of the test:

  1. pypy-pinned env test failed because of an explicit PyPyDispatcher == 2.1.0 deps
  2. pypy env test failed because PyDispatcher==2.0.5 is not explicitly declared
  3. asyncio-pinned test failed because of a timeout. Using a new version of Twisted fixed this by having pip install additional packages (async-timeout , ...). Not sure why the old resolver passed this test.

Although running pip list before pytest shows the exact same packages installed on old and new pip resolver. Unsure on what is causing this, but I think it'll be better having two separate test for the new and old resolver for now. (since pip themselves suggest using it on a non-production environment only)

The more feedback we can get, the more we can make sure that the final release is solid. (Only try the new resolver in a non-production environment, though - it isn’t ready for you to rely on in production!)

from here

@Gallaecio
Copy link
Member

I think we need to figure out why the PyPyDispatcher thing is not working.

From the PyPyDispatcher README, I see it’s meant as a standalone replacement for PyDispatcher with PyPy support, so the first thing I would try is to prevent PyDispatcher from being installed on PyPy; I suspect the issue may be that it gets imported instead of the PyPy replacement.

I think we should edit the setup.py file to require PyDispatcher outside PyPy and PyPyDispatcher under PyPy.

.travis.yml Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@Gallaecio
Copy link
Member

As for the asyncio job getting stuck with Twisted==17.9.0, I’ve found that the offending test is tests/test_downloadermiddleware.py::MiddlewareUsingCoro::test_asyncdef. @wRAR Do you think you can have a look? Maybe we need to document a higher minimum version of Twisted when using asyncio? Or there’s some Twisted bug that we may be able to workaround?

@wRAR
Copy link
Member

wRAR commented Oct 8, 2020

I was able to reproduce the difference in the test behavior locally, I will look further into it.

@wRAR
Copy link
Member

wRAR commented Oct 9, 2020

Not sure if it's already known, but with the old resolver the newest Twisted gets installed even in the pinned env :) See e.g. https://travis-ci.org/github/scrapy/scrapy/jobs/734210368

@wRAR
Copy link
Member

wRAR commented Oct 9, 2020

The actual problem is fixed in 18.4.0 and I suspect it's because of http://twistedmatrix.com/trac/ticket/9390 (which is probably specific to tests?)

@dswij
Copy link
Contributor Author

dswij commented Oct 9, 2020

Not sure if it's already known, but with the old resolver the newest Twisted gets installed even in the pinned env :) See e.g. https://travis-ci.org/github/scrapy/scrapy/jobs/734210368

It appears that only Twisted has version mismatch in declared deps and installed version. I suspect that the new resolver might install older version only because newer versions of Twisted has conflict with pinned zope.interface versions. The new resolver resorted to the specified version of Twisted in deps to resolve the conflict.

@dswij
Copy link
Contributor Author

dswij commented Oct 10, 2020

It appears that only Twisted has version mismatch in declared deps and installed version. I suspect that the new resolver might install older version only because newer versions of Twisted has conflict with pinned zope.interface versions. The new resolver resorted to the specified version of Twisted in deps to resolve the conflict.

Turns out using an old version of Twisted and newer version of zope.interface still enforces pinned version of Twisted in new resolver. So this is not the case.

@Gallaecio
Copy link
Member

Thanks. Yes, upgrading those 2 works.

However, I think we might want to consider the opposite approach: skip tests required mitmproxy in pinned environments instead. Because the point of pinned environments is that everything works with the minimum version, I think that testing that most tests pass with all the minimum versions is more valuable than testing that all tests pass with most of the minimum versions.

@wRAR
Copy link
Member

wRAR commented Oct 30, 2020

If installing mitmproxy leads to upgrading our deps, agreed.

tox.ini Outdated Show resolved Hide resolved
@Gallaecio Gallaecio closed this Nov 18, 2020
@Gallaecio Gallaecio reopened this Nov 18, 2020
@Gallaecio Gallaecio merged commit 39bc9a4 into scrapy:master Dec 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted This pull request looks valid for Hackoctoberfest even if it has not been approved yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test the upcoming pip dependency resolver in CI
5 participants