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
Contributor

@ilevkivskyi ilevkivskyi commented Nov 19, 2018

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 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
Contributor

@ilevkivskyi ilevkivskyi commented Nov 19, 2018

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

Great, thanks for looking into this!

@dstufft
Copy link
Member

@dstufft dstufft commented Nov 19, 2018

@dstufft
Copy link
Member

@dstufft dstufft commented Nov 19, 2018

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

@serhiy-storchaka serhiy-storchaka force-pushed the serhiy-storchaka:collections-abc branch from c1ffdbc to 6e1d424 Nov 19, 2018
@tirkarthi
Copy link
Member

@tirkarthi tirkarthi commented Nov 21, 2018

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

@serhiy-storchaka serhiy-storchaka commented Dec 20, 2018

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

@tirkarthi tirkarthi commented Dec 20, 2018

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

@tirkarthi tirkarthi commented Dec 20, 2018

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

@rhettinger rhettinger commented Dec 23, 2018

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 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

@tirkarthi tirkarthi commented Jan 22, 2019

@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
Contributor

@pradyunsg pradyunsg commented Feb 4, 2019

(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 serhiy-storchaka force-pushed the serhiy-storchaka:collections-abc branch from 6e1d424 to 95262b1 Mar 5, 2019
@serhiy-storchaka
Copy link
Member Author

@serhiy-storchaka serhiy-storchaka commented Mar 7, 2019

pip 19.0.3 still is not compatible with this PR.

@pradyunsg
Copy link
Contributor

@pradyunsg pradyunsg commented Mar 7, 2019

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 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.

@yan12125
Copy link
Contributor

@yan12125 yan12125 commented Aug 12, 2019

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

@sersorrel sersorrel commented Aug 14, 2019

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 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
Contributor

@pradyunsg 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 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
4 checks passed
4 checks passed
Azure Pipelines PR #20191005.9 succeeded
Details
bedevere/issue-number Issue number 25988 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:collections-abc branch Oct 7, 2019
@pradyunsg
Copy link
Contributor

@pradyunsg pradyunsg commented Oct 7, 2019

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

jacobneiltaylor added a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
dongjoon-hyun added 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 to vstinner/cpython that referenced this pull request Feb 18, 2020
…es in the collections module. (pythonGH-10596)"

This reverts commit ef092fe.

Update collections __getattr__() and documentation to defer aliases
removal to Python 3.10.
vstinner added a commit to vstinner/cpython that referenced this pull request Feb 18, 2020
…es in the collections module. (pythonGH-10596)"

This reverts commit ef092fe.

Update collections __getattr__() and documentation to defer aliases
removal to Python 3.10.
vstinner added a commit to vstinner/cpython that referenced this pull request Feb 18, 2020
…es in the collections module. (pythonGH-10596)"

This reverts commit ef092fe.

Update collections __getattr__() and documentation to defer aliases
removal to Python 3.10.
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.
yingshikong186 pushed a commit to yingshikong186/spark that referenced this pull request Apr 14, 2020
…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 apache#26835 from tirkarthi/spark-30205-fix-abc-warnings.

Authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.