[Django] #27818: Use contextlib.suppress to suppress exceptions.
Django
------------------------------------------------+------------------------
Reporter: Mads Jensen | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
As per a comment in #23919, this pattern can be replaced with
`contextlib.suppress(...)` which is valid as of Python 3.4. The code looks
a bit cleaner.
{{{
try:
...
except ...:
pass
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27818>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Mads Jensen | Owner:
Type: | ChillarAnand
Cleanup/optimization | Status: assigned
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by ChillarAnand):
* status: new => assigned
* owner: nobody => ChillarAnand
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:1>
Django
Reporter: Mads Jensen | Owner:
Type: | ChillarAnand
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham):
* component: Uncategorized => Core (Other)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:2>
Django
Reporter: Mads Jensen | Owner: Mads
Type: | Jensen
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Mads Jensen):
* owner: ChillarAnand => Mads Jensen
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:3>
Django
Reporter: Mads Jensen | Owner: Mads
Type: | Jensen
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Comment (by Claude Paroz):
Please link to the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:4>
Django
Reporter: Mads Jensen | Owner: Mads
Type: | Jensen
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mads Jensen):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:4>
Django
Reporter: Mads Jensen | Owner: Nithun
Type: | Harikrishnan
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Nithun Harikrishnan):
* owner: Mads Jensen => Nithun Harikrishnan
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:5>
Django
Reporter: Mads Jensen | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Nithun Harikrishnan):
* owner: Nithun Harikrishnan => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:6>
Django
Reporter: Mads Jensen | Owner: Ratnadeep
Type: | Debnath
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Ratnadeep Debnath):
* status: new => assigned
* owner: Mads Jensen => Ratnadeep Debnath
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:5>
Django
Reporter: Mads Jensen | Owner: (none)
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by rtnpro):
* owner: rtnpro => (none)
* status: assigned => new
Comment:
Sorry, I assigned it to myself, by mistake.
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:6>
Django
Reporter: Mads Jensen | Owner: Phil
Type: | Bazun
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Phil Bazun):
* status: new => assigned
* owner: (none) => Phil Bazun
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:7>
Django
Reporter: Mads Jensen | Owner: (none)
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by Tim Graham):
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:5>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham <timograham@…>):
* owner: (none) => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"550cb3a365dee4edfdd1563224d5304de2a57fda" 550cb3a]:
{{{
#!CommitTicketReference repository=""
revision="550cb3a365dee4edfdd1563224d5304de2a57fda"
Fixed #27818 -- Replaced try/except/pass with contextlib.suppress().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:6>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) Johnson):
I only found out about this in review from Tim the other week. I think it
should be reverted because it's making things slower unnecessarily. I
count contextlib.suppress as about 10 times slower than plain try/except,
not to mention it can trigger garbage collection as it creates and
destroys an object every time:
{{{
In [8]: def foo():
...: try:
...: pass
...: except AttributeError:
...: pass
...:
In [9]: def foo2():
...: with contextlib.suppress(AttributeError):
...: pass
...:
In [10]: %timeit foo()
The slowest run took 10.24 times longer than the fastest. This could mean
that an intermediate result is being cached.
10000000 loops, best of 3: 111 ns per loop
In [11]: %timeit foo2()
The slowest run took 6.15 times longer than the fastest. This could mean
that an intermediate result is being cached.
1000000 loops, best of 3: 1.13 µs per loop
}}}
How would we proceed? Should I take this to the mailing list, or can I
just submit a revert PR?
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:7>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
If a change slows down things, please revert the change.
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:8>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I'm not against reverting, but before doing so, it could be useful to ask
cpython developers if the slowness should be a documented caveat of
`contextlib.suppress()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:9>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I got this input from #python-dev:
`Alex_Gaynor`: Without measuring, I'd bet almost anything that the
majority of the overhead is in an additional function call, so it's not
really avoidable. I'm not sure it makes sense to docuemnt, _any_ fucntion
call would add the same overhead (assuming I'm right). (For that reason
I'd expect it to have no additional overhead on PyPy)
` __ap__`: Those kinds of wholesale replacements look like an anti-pattern
to me. Most of the time, the un-abstracted version is as readable as the
slower abstracted one.
[https://github.com/django/django/pull/9038 PR] to revert.
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:10>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Adam (Chainz) Johnson (added)
Comment:
Thanks Tim for checking with #python-dev and writing the PR! I forgot to
add myself to CC so didn't get email updates (I'm used to systems where
comment = auto subscribe).
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:11>
Django
Reporter: Mads Jensen | Owner: Tim
Type: | Graham <timograham@…>
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"6e4c6281dbb7ee12bcdc22620894edb4e9cf623f" 6e4c6281]:
{{{
#!CommitTicketReference repository=""
revision="6e4c6281dbb7ee12bcdc22620894edb4e9cf623f"
Reverted "Fixed #27818 -- Replaced try/except/pass with
contextlib.suppress()."
This reverts commit 550cb3a365dee4edfdd1563224d5304de2a57fda
because try/except performs better.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27818#comment:12>