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

bpo-25988: Do not expose abstract collection classes in the collections module. #10596

Merged
merged 6 commits into from
Oct 7, 2019

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Nov 19, 2018

@ilevkivskyi
Copy link
Member

I think it makes sense to ping pip folks about the test failure. @pfmoore @dstufft not something urgent, since 3.8 is still far away, but it would be great if pip didn't use the deprecated collections imports like collections.Mapping and switched to collections.abc.Mapping instead, thanks!.

@dstufft
Copy link
Member

dstufft commented Nov 19, 2018

This is actually in urllib3, we just bundle urllib3, it looks fixed in urllib3 and in pip and we just need to update CPython's bundled copy of pip.

@ilevkivskyi
Copy link
Member

we just need to update CPython's bundled copy of pip.

Great, thanks for looking into this!

@dstufft
Copy link
Member

dstufft commented Nov 19, 2018

#10598

@dstufft
Copy link
Member

dstufft commented Nov 19, 2018

The above has been merged, you should be able to rebase this PR.

@tirkarthi
Copy link
Member

This might cause make doctest to fail since some of the build dependencies might be incompatible https://bugs.python.org/issue35109

cc: @JulienPalard

@serhiy-storchaka
Copy link
Member Author

Tests still are failing because of incompatibility in html5lib bundled with pip.

  File "<frozen zipimport>", line 259, in load_module
  File "/tmp/tmpcbwhlouc/pip-18.1-py2.py3-none-any.whl/pip/_vendor/html5lib/_trie/_base.py", line 3, in <module>
ImportError: cannot import name 'Mapping' from 'collections' (/home/travis/build/python/cpython/Lib/collections/__init__.py)

Note also syntax warnings produced by requests.

/tmp/tmpocpc284j/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:18: SyntaxWarning: invalid escape sequence \o

@tirkarthi
Copy link
Member

Warning in html5lib's _trie/_base.py seems to have been fixed upstream : html5lib/html5lib-python#403

Upstream issue I opened while testing : html5lib/html5lib-python#402

@tirkarthi
Copy link
Member

Requests also seem to be fixed : psf/requests#4728

So I guess pip's vendor folder needs to be updated for both libraries?

@rhettinger
Copy link
Contributor

FWIW, it would be reasonable to delay this be one additional point release.

Also, if there are known major projects that still do the old imports, we should submit bug reports to those projects (I've already done some of these).

@tirkarthi
Copy link
Member

tirkarthi commented Dec 23, 2018

Also, if there are known major projects that still do the old imports,

Our docs build had this problem with a dependency that had this issue and now it's fixed upstream. I think it's better to do it now that gives more time and will help users and libraries who test their code with alpha and beta candidates if they have these imports.

@tirkarthi
Copy link
Member

@dstufft @pradyunsg https://bugs.python.org/issue35807 updates pip to 19.0 but I still see the vendored copy using html5lib that has this warning. Does pip update vendored version copy only after a release from html5lib? Should I file an issue in Pip's repo for this?

@pradyunsg
Copy link
Member

(sorry for the delay in responding; swamped in GitHub notifications currently)

Does pip update vendored version copy only after a release from html5lib?

Yep. pip downloads and vendors only versions released on PyPI.

Should I file an issue in Pip's repo for this?

This isn't necessary since we basically do a round of vendoring updates before a release which should cover this, but yeah, it won't hurt either. Please feel free to.

@serhiy-storchaka
Copy link
Member Author

pip 19.0.3 still is not compatible with this PR.

@pradyunsg
Copy link
Member

Yea, there hasn't been an update for html5lib yet. :(

I pinged the author on #pypa-dev on Freenode today about this.

@methane
Copy link
Member

methane commented Mar 27, 2019

If we postpone the removal, can we change warning class from DeprecationWarning to ImportWarning to nosy more people?

Maybe, many Python developers doesn't care about DeprecationWarning after Python 3.2.

@pradyunsg
Copy link
Member

Ah. So that explains the documentation build failing. Thanks for the super quick response to a trivial question!

@yan12125
Copy link
Contributor

yan12125 commented Aug 5, 2019

I gave it a try - the jinja issue is the last blocker. If I use an unreleased commit of jinja [1], tests are green [2].

[1] https://github.com/yan12125/cpython/commit/e6641707fb87774bf6359d8a584047cc94cc5b68#diff-c8dee9c3c068ad3447bbf459950a35b5
[2] https://travis-ci.org/yan12125/cpython/builds/567688247

@vstinner
Copy link
Member

The issue was fixed in 2.10.2 more than a year ago, but this version is not released yet.

That's surprising. Why was a tag created in Git with no public release on PyPI?

Jinja2 fix for collections.abc: pallets/jinja@593ee1e

@yan12125
Copy link
Contributor

Seems 2.10.2 is not tagged yet (or it was tagged but later removed): https://github.com/pallets/jinja/tags

@sersorrel
Copy link
Contributor

There was an issue filed against Jinja2 in April asking when the fix would be included in a release, but the maintainers aren't saying much: pallets/jinja#973

@yan12125
Copy link
Contributor

yan12125 commented Oct 4, 2019

A new version of Jinja is out - http://jinja.palletsprojects.com/en/2.10.x/changelog/#version-2-10-2. This pull request can go forward now.

And the git conflict seems trivial to fix :) c4106af

A regression is discovered in Jinja 2.10.2 (pallets/jinja#1069). It might be better to wait for 2.10.3.

@pradyunsg
Copy link
Member

pradyunsg commented Oct 4, 2019

The new (2.10.3) jinja release has been made! We're unblocked on this! :)

@yan12125
Copy link
Contributor

yan12125 commented Oct 5, 2019

I gave it another try - with this PR rebased [1] and Jinja 2.10.3 installed, tests are still green [2].

[1] https://github.com/yan12125/cpython/commits/collections-abc (branch removed as this PR has been merged)
[2] https://travis-ci.org/yan12125/cpython/builds/593814893

@serhiy-storchaka serhiy-storchaka merged commit ef092fe into python:master Oct 7, 2019
@serhiy-storchaka serhiy-storchaka deleted the collections-abc branch October 7, 2019 09:10
@pradyunsg
Copy link
Member

Yay! We managed to do this in under an year! ^>^

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Dec 10, 2019
…recation warnings

### What changes were proposed in this pull request?

This PR aims to remove deprecation warnings by importing ABCs from `collections.abc` instead of `collections`.
- python/cpython#10596

### Why are the changes needed?

This will remove deprecation warnings in Python 3.7 and 3.8.

```
$ python -V
Python 3.7.5

$ python python/pyspark/resultiterable.py
python/pyspark/resultiterable.py:23: DeprecationWarning:
Using or importing the ABCs from 'collections' instead of from 'collections.abc'
is deprecated since Python 3.3,and in 3.9 it will stop working
  class ResultIterable(collections.Iterable):
```

### Does this PR introduce any user-facing change?

No, this doesn't introduce user-facing change

### How was this patch tested?

Manually because this is about deprecation warning messages.

Closes #26835 from tirkarthi/spark-30205-fix-abc-warnings.

Authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
vstinner added a commit that referenced this pull request Feb 18, 2020
…es in the collections module. (GH-10596)" (GH-18545)

This reverts commit ef092fe.

Update collections __getattr__() and documentation to defer aliases
removal to Python 3.10.
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.

None yet