add cchardet support and use chardet as fallback by 2mf · Pull Request #4115 · psf/requests

2mf · 2017-05-30T14:21:56Z

I created this pull-request for #4112

sethmlarson

There's a lot of duplicated logic in your change request. Can any of it be de-duplicated?

Lukasa

Yeah, we need to try to deduplicate this logic a lot. Do you mind doing that?

2mf · 2017-05-30T14:26:57Z

sure :-)
I have to update the test settings as well to ensure chardet and cchardet run both

sethmlarson · 2017-05-30T14:28:02Z

requests/packages.py

+    if isinstance(_package, tuple):
+        package = _package[1]
+        try:
+            locals()[package] = __import__(package[0])


Doesn't look like this line is correct, should be __import__(_package[0])? Might want to unpack the tuple right away and then use proper variable names so the logic is easier to follow.

Lukasa

There's an issue here with the warning that we're raising.

Lukasa · 2017-05-30T16:34:30Z

requests/__init__.py

+        assert minor >= 1
+        assert patch >= 0
+    except AssertionError:
+        warnings.warn('Requests dependency \'cchardet\' must be version >= 2.1.0! Falling back to chardet')


This isn't specifying the upper pin, which is < 3.0.0.

nateprewitt

Hey @2mf, thanks for putting this together! While I'm not opposed to supporting cchardet, I do have some questions/comments on the current implementation.

nateprewitt · 2017-05-30T15:59:50Z

tox.ini

 [testenv]

+deps =
+    py{26,27,33,34,35,36}-chardet: chardet


I'm not super fond of doubling the number of test instances we're running.

I'm not quite sure, how I can test against ccharset and charset using existing cases

nateprewitt · 2017-05-30T16:32:56Z

requests/__init__.py


+import warnings
+
+def get_version(package):


If we are going to abstract this out to a function, maybe we add it to the urllib3 import to reduce the duplicated code to a single place?

sure I'll do that

nateprewitt · 2017-05-30T16:36:06Z

requests/__init__.py

+        warnings.warn('Requests dependency \'cchardet\' must be version >= 2.1.0! Falling back to chardet')
+        raise
+
+except (ImportError, SyntaxError, AssertionError) as e:


What's the scenario that raises a SyntaxError? Also if we're not using e for anything, we probably don't need to set it.

I copied that from other places in requests, but from my view point it is not needed

kennethreitz · 2017-05-30T16:43:56Z

requests/packages.py

-    locals()[package] = __import__(package)
+for package in ('urllib3', 'idna', ('chardet', 'cchardet')):
+    if isinstance(package, tuple):
+        package, alt_package = package


we need to be extremely careful about touching this code, at all.

I agree. This looks safe, but it'd be so much better if this were testable.

kennethreitz · 2017-05-30T16:45:11Z

This whole thing scares me.

Lukasa · 2017-05-30T16:45:54Z

I for one am not wedded to doing it: I'm nervous about the amount of code it adds as well.

kennethreitz · 2017-05-30T16:52:09Z

@2mf do you have any metrics on how much faster cchardet is than chardet? e.g. is it worth the trouble?

2mf · 2017-05-30T17:36:18Z

@kennethreitz a benchmark can be seen at https://pypi.python.org/pypi/cchardet

Python 2.7.12
Request (call/s)
chardet 0.26
cchardet 1341.81

Python 3.6.0
Request (call/s)
chardet 0.26
cchardet 1472.43

2mf · 2017-05-30T17:42:52Z

@kennethreitz @Lukasa
If you both don't like the idea of including cchardet. I don't have a problem with that but then there must be a possibility to use cchardet by monkey patching but this is prevented by other things like:

shouldn't be the version matching in the requirements of setup.py?

# Check chardet for compatibility.
import chardet
major, minor, patch = chardet.__version__.split('.')[:3]
major, minor, patch = int(major), int(minor), int(patch)
# chardet >= 3.0.2, < 3.1.0
try:
    assert major == 3
    assert minor < 1
    assert patch >= 2
except AssertionError:
    raise RuntimeError('Requests dependency \'chardet\' must be version >= 3.0.2, < 3.1.0!')

and

for package in ('urllib3', 'idna', 'chardet'):
    locals()[package] = __import__(package)
    # This traversal is apparently necessary such that the identities are
    # preserved (requests.packages.urllib3.* is urllib3.*)
    for mod in list(sys.modules):
        if mod == package or mod.startswith(package + '.'):
            sys.modules['requests.packages.' + mod] = sys.modules[mod]

kennethreitz · 2017-05-30T17:44:10Z

That is significantly faster!

remove unnecessary SyntaxError pin cchardet to >=2.1.0 <2.2.0

This reverts commit b231590.

sigmavirus24 · 2017-05-30T18:23:30Z

shouldn't be the version matching in the requirements of setup.py?

No, @2mf it shouldn't. Requests needs very specific versions of dependencies. Some of those dependencies are popular and we need a way to tell the user "You're using a version that will horribly break your version of Requests"

sigmavirus24 · 2017-05-30T18:24:47Z

requests/__init__.py

+        assert minor < 2
+        assert patch >= 0
+    except AssertionError:
+        warnings.warn('Requests dependency \'cchardet\' must be version >= 2.1.0, < 2.2.0! Falling back to chardet')


This is not a dependency of Requests, it's an optional speed-up. Please revise

sigmavirus24 · 2017-05-30T18:25:04Z

requests/__init__.py

+    # cchardet >= 2.1.0
+    try:
+        assert major == 2
+        assert minor >= 1


Why not:

assert 1 <= minor < 2

sigmavirus24 · 2017-05-30T18:26:29Z

requests/__init__.py

+except (ImportError, AssertionError):
+    # Check chardet for compatibility.
+    import chardet
+    major, minor, patch = get_version(chardet)


chardet shouldn't be imported in this except block. This will cause extremely confusing tracebacks for users if they can't import either.

sigmavirus24 · 2017-05-30T18:28:24Z

setup.py

    cmdclass={'test': PyTest},
    tests_require=test_requirements,
    extras_require={
+        'cchardet:(python_version == "2.7" or python_version >= "3.4")': ['cchardet>=2.1.0'],


If we're enforcing (in __init__.py) that cchardet be between 2.1.0 and 2.2.0 then that should be written here.

sigmavirus24 · 2017-05-30T18:29:23Z

requests/packages.py

-    locals()[package] = __import__(package)
+for package in ('urllib3', 'idna', ('chardet', 'cchardet')):
+    if isinstance(package, tuple):
+        package, alt_package = package


I agree. This looks safe, but it'd be so much better if this were testable.

sigmavirus24 · 2017-05-30T18:34:47Z

That is significantly faster!

And harder to install and the benchmark, as I understand it, is based off a very old version of chardet and hasn't been updated since.

kennethreitz · 2017-05-30T18:36:17Z

It sounds like we're against this change overall. Sorry @2mf.

sigmavirus24 · 2017-05-30T19:04:39Z

I'm on board with doing this, but the current methodology isn't 100% correct and I'm merely pointing out that those benchmarks are older and potentially misleading. @dan-blanchard has done a lot of fantastic work on chardet recently

dan-blanchard · 2017-05-30T19:08:07Z

I feel the need to point out that chardet is in the midst of gaining support for more codecs and languages than cchardet supports. I'm retraining all the models for greater accuracy too. Speed isn't everything with encoding detection.

2mf · 2017-05-30T19:45:53Z

Just for the sake of a recent benchmark
I did the same benchmark for chardet and cchardet that where done by @PyYoshi (on my machine) and the difference is still in the same dimension.

Python 3.6.1
chardet 3.0.3
cchardet 2.1.0

python test_chardet.py
chardet: 0.24908731023612882 call(s)/s

python test_cchardet.py
chardet: 1161.9214360906421 call(s)/s

anyway, It is not worth to continue on this one while running against so many walls

dan-blanchard · 2017-05-30T19:56:57Z

I hadn't actually looked at the benchmark code cChardet uses before, but it just runs the same small SJIS text file through both modules 100 times. I know cChardet will pretty much always be faster than chardet—in fact, if you look at old chardet issues, you'll even see that I once toyed with the idea of trying to merge the projects—but that is not an accurate benchmark, especially for something like requests where a good amount of the time is spent downloading the page content anyway. A requests-based benchmark would make sense for deciding if it makes a difference here.

add cchardet and use chardet as fallback

689b692

sethmlarson requested changes May 30, 2017

View reviewed changes

Lukasa requested changes May 30, 2017

View reviewed changes

sethmlarson requested changes May 30, 2017

View reviewed changes

2mf added 4 commits May 30, 2017 17:16

improve readability

2335dae

update extra require with limitations for cchardet

fbe5a7a

update tox.ini

b231590

dedupe logic

2f88d52

Lukasa requested changes May 30, 2017

View reviewed changes

nateprewitt requested changes May 30, 2017

View reviewed changes

kennethreitz reviewed May 30, 2017

View reviewed changes

2mf added 2 commits May 30, 2017 19:57

use get_version as well for urllib3

0e3bb27

remove unnecessary SyntaxError pin cchardet to >=2.1.0 <2.2.0

Revert "update tox.ini"

03851c5

This reverts commit b231590.

sigmavirus24 requested changes May 30, 2017

View reviewed changes

kennethreitz closed this May 30, 2017

kennethreitz reopened this May 30, 2017

2mf closed this May 30, 2017

ashb mentioned this pull request Apr 22, 2021

Switch LGPL'd chardet for MIT licensed charset_normalizer #5797

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2021

Uh oh!

Conversation

2mf commented May 30, 2017

Uh oh!

sethmlarson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

2mf commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nateprewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennethreitz commented May 30, 2017

Uh oh!

Lukasa commented May 30, 2017

Uh oh!

kennethreitz commented May 30, 2017

Uh oh!

2mf commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2mf commented May 30, 2017

Uh oh!

kennethreitz commented May 30, 2017

Uh oh!

sigmavirus24 commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigmavirus24 commented May 30, 2017

Uh oh!

kennethreitz commented May 30, 2017

Uh oh!

sigmavirus24 commented May 30, 2017

Uh oh!

dan-blanchard commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2mf commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dan-blanchard commented May 30, 2017

Uh oh!

Reviewers

Assignees

sethmlarson left a comment •

edited

Loading

2mf commented May 30, 2017 •

edited

Loading

dan-blanchard commented May 30, 2017 •

edited

Loading

2mf commented May 30, 2017 •

edited

Loading