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

support chardet v5.0.0 #6177

Closed
copdips opened this issue Jun 25, 2022 · 6 comments
Closed

support chardet v5.0.0 #6177

copdips opened this issue Jun 25, 2022 · 6 comments

Comments

@copdips
Copy link

copdips commented Jun 25, 2022

chardet just released v5.0.0 which is not supported by the latest version (2.28.0) of requests:

assert (3, 0, 2) <= (major, minor, patch) < (5, 0, 0)

Could you please add the support for the v5.0.0 of chardet ?

@sanderr
Copy link

sanderr commented Jun 27, 2022

Related to this issue, I'm wondering about why chardet is used at all in this case. If I understand correctly, if chardet can be imported it is always preferred over charset_normalizer, even if requests was not installed with the use_chardet_on_py3 extra, which could happen if another package includes it as a dependency.
I understand it might not be straightforward to check whether the package was installed with the extra (perhaps through an empty meta package?), but at the least I would expect that if the version is incompatible, charset_normalizer is used instead. Because either this means requests was not installed with the extra (and chardet was not meant to be used), or there is an actual dependency conflict which could be discovered with the help of pip check.

@jepler
Copy link

jepler commented Jun 27, 2022

This is affecting us as well over at https://github.com/adafruit/cookiecutter-adafruit-circuitpython and possibly other projects that use requests together with some other package that installs chardet.

Here's a 'reproducer' for the problem (in our case, cookiecutter depends on binaryornot depends on chardet) :

$ python3 -mvenv _env
$ . _env/bin/activate
$ pip install requests binaryornot
$ python3 -c 'import requests'

At the last command, this warning is produced:

/tmp/_env/lib/python3.9/site-packages/requests/__init__.py:109: RequestsDependencyWarning: urllib3 (1.26.9) or chardet (5.0.0)/charset_normalizer (2.0.12) doesn't match a supported version!
  warnings.warn(

When importing requests under pytest, it appears that the caught AssertionError and the RequestsDependencyWarning are treated as errors, as seen in this workflow excerpt:

 _________________ ERROR collecting tests/test_make_cookies.py __________________
../../../hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/requests/__init__.py:106: in <module>
    urllib3.__version__, chardet_version, charset_normalizer_version
../../../hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/requests/__init__.py:79: in check_compatibility
    assert (3, 0, 2) <= (major, minor, patch) < (5, 0, 0)
E   AssertionError
During handling of the above exception, another exception occurred:
tests/test_make_cookies.py:11: in <module>
    from cookiecutter.main import cookiecutter
../../../hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/cookiecutter/main.py:17: in <module>
    from cookiecutter.repository import determine_repo_dir
../../../hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/cookiecutter/repository.py:7: in <module>
    from cookiecutter.zipfile import unzip
../../../hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/cookiecutter/zipfile.py:6: in <module>
    import requests
../../../hostedtoolcache/Python/3.7.13/x64/lib/python3.7/site-packages/requests/__init__.py:114: in <module>
    RequestsDependencyWarning,
E   requests.exceptions.RequestsDependencyWarning: urllib3 (1.26.9) or chardet (5.0.0)/charset_normalizer (2.0.12) doesn't match a supported version!
=========================== short test summary info ============================
ERROR tests/test_make_cookies.py - requests.exceptions.RequestsDependencyWarn...
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.60s ===============================
Error: Process completed with exit code 2.

.. this makes our CI process fail.

@nateprewitt
Copy link
Member

We've merged #6179 which should address this. We could potentially remove the chardet check now that it's not the primary character detector but we're introducing some risk. Chardet major versions maybe every 2 years, sometimes longer. If users are co-installing chardet from somewhere else, Requests currently uses it, so we should continue to be clear if there's an incompatible version being used.

There's been discussion of removing chardet entirely in previous issues (#5871) which will likely happen eventually, but I don't foresee us changing that in the immediate future. I believe our current change should mitigate issues for the immediate future though. The fix will be available in the next patch release 2.28.1.

@sanderr
Copy link

sanderr commented Jun 28, 2022

What about the suggestion to use charset_normalizer if chardet is installed but has an incompatible version?

inmantaci pushed a commit to inmanta/inmanta-core that referenced this issue Jun 28, 2022
…n restored (PR #4467)

# Description

With the context given in psf/requests#6177 (comment), I think it should be safe to drop this requirement again. Do you agree?

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Type annotations are present
- [ ] Code is clear and sufficiently documented
- [ ] No (preventable) type errors (check using make mypy or make mypy-diff)
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

# Reviewer Checklist:

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
inmantaci pushed a commit to inmanta/inmanta-core that referenced this issue Jun 28, 2022
…n restored (PR #4467)

# Description

With the context given in psf/requests#6177 (comment), I think it should be safe to drop this requirement again. Do you agree?

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Type annotations are present
- [ ] Code is clear and sufficiently documented
- [ ] No (preventable) type errors (check using make mypy or make mypy-diff)
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

# Reviewer Checklist:

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
@nateprewitt
Copy link
Member

What about the suggestion to use charset_normalizer if chardet is installed but has an incompatible version?

While this avoids a warning every couple years, it introduces subtle failure modes. When a new major version is released and a user unintentionally installs it, they've potentially disabled an opt-in they made previously. This results in impact on their system(s) they weren't intending and we get a long list of issues. Being explicit is what we've found to be the least problematic over the lifetime of the library.

@sanderr
Copy link

sanderr commented Jun 29, 2022

I see, thanks for the detailed response. Making it even more explicit by detecting whether the extra was actually selected (making it opt-in only) would probably be the best of both worlds, but if there's talk of removing chardet entirely at some point I guess there's no sufficient incentive to put in that effort.

HippocampusGirl added a commit to HALFpipe/HALFpipe that referenced this issue Jun 29, 2022
- Due to incompatibility with `requests`, see
  psf/requests#6177
arnaudsjs pushed a commit to inmanta/inmanta-core that referenced this issue Jul 28, 2022
…n restored (PR #4467)

With the context given in psf/requests#6177 (comment), I think it should be safe to drop this requirement again. Do you agree?

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Type annotations are present
- [ ] Code is clear and sufficiently documented
- [ ] No (preventable) type errors (check using make mypy or make mypy-diff)
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
arnaudsjs pushed a commit to inmanta/inmanta-core that referenced this issue Jul 28, 2022
…n restored (PR #4467)

With the context given in psf/requests#6177 (comment), I think it should be safe to drop this requirement again. Do you agree?

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Type annotations are present
- [ ] Code is clear and sufficiently documented
- [ ] No (preventable) type errors (check using make mypy or make mypy-diff)
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
arnaudsjs pushed a commit to inmanta/inmanta-core that referenced this issue Jul 28, 2022
…n restored (PR #4467)

With the context given in psf/requests#6177 (comment), I think it should be safe to drop this requirement again. Do you agree?

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [ ] Attached issue to pull request
- [ ] Changelog entry
- [ ] Type annotations are present
- [ ] Code is clear and sufficiently documented
- [ ] No (preventable) type errors (check using make mypy or make mypy-diff)
- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants