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

Consider deprecating UP027 or improve its docs #12754

Closed
sk- opened this issue Aug 8, 2024 · 8 comments · Fixed by #12843
Closed

Consider deprecating UP027 or improve its docs #12754

sk- opened this issue Aug 8, 2024 · 8 comments · Fixed by #12843
Assignees
Labels
rule Implementing or modifying a lint rule
Milestone

Comments

@sk-
Copy link

sk- commented Aug 8, 2024

The original rule UP027 (unpacked-list-comprehension), implemented in pyupgrade does not seem to be thoroughly justified, and in fact it is actually slower. Which goes against the documentation in ruff which mentions

which is more efficient as it avoids allocating an intermediary list

I raised a question in pyupgrade, showing how using generators is actually 3 times slower than unpacking a list comprehension

System
Python 3.12.4 (main, Jun 6 2024, 18:26:44) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Mac OS Sonoma 14.5

$ python -m timeit "a,b = [i for i in range(2)]"
5000000 loops, best of 5: 78.3 nsec per loop
$ python -m timeit "a,b,c = [i for i in range(3)]"
5000000 loops, best of 5: 86.2 nsec per loop
$ python -m timeit "a,b,c,d = [i for i in range(4)]"
5000000 loops, best of 5: 92.7 nsec per loop
$ python -m timeit "a,b,c,d,e = [i for i in range(5)]"
2000000 loops, best of 5: 114 nsec per loop

$ python -m timeit "a,b = (i for i in range(2))"
1000000 loops, best of 5: 225 nsec per loop
$ python -m timeit "a,b,c = (i for i in range(3))"
1000000 loops, best of 5: 263 nsec per loop
$python -m timeit "a,b,c,d = (i for i in range(4))"
1000000 loops, best of 5: 286 nsec per loop
$ python -m timeit "a,b,c,d,e = (i for i in range(5))"
1000000 loops, best of 5: 321 nsec per loop
@charliermarsh
Copy link
Member

@carljm -- Do you have an opinion here?

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Aug 9, 2024
@trim21
Copy link
Contributor

trim21 commented Aug 9, 2024

should we have a reversed version as perf rule?

@shoucandanghehe
Copy link

Hi guys, I did some small tests and the results show that in python 3.6-3.12 list comprehension are faster than generator

Python Version Elements List Comprehension (nsec per loop) Generator (nsec per loop) Difference (nsec)
3.6.13 2 315 nsec 364 nsec +49 nsec
3 339 nsec 402 nsec +63 nsec
4 368 nsec 438 nsec +70 nsec
5 407 nsec 475 nsec +68 nsec
3.7.16 2 287 nsec 350 nsec +63 nsec
3 321 nsec 387 nsec +66 nsec
4 341 nsec 412 nsec +71 nsec
5 374 nsec 455 nsec +81 nsec
3.8.19 2 269 nsec 321 nsec +52 nsec
3 298 nsec 356 nsec +58 nsec
4 324 nsec 391 nsec +67 nsec
5 356 nsec 423 nsec +67 nsec
3.9.19 2 254 nsec 296 nsec +42 nsec
3 287 nsec 328 nsec +41 nsec
4 304 nsec 358 nsec +54 nsec
5 345 nsec 385 nsec +40 nsec
3.10.14 2 272 nsec 317 nsec +45 nsec
3 296 nsec 353 nsec +57 nsec
4 327 nsec 382 nsec +55 nsec
5 356 nsec 408 nsec +52 nsec
3.11.9 2 240 nsec 276 nsec +36 nsec
3 264 nsec 301 nsec +37 nsec
4 278 nsec 335 nsec +57 nsec
5 320 nsec 351 nsec +31 nsec
3.12.4 2 112 nsec 200 nsec +88 nsec
3 122 nsec 219 nsec +97 nsec
4 136 nsec 235 nsec +99 nsec
5 155 nsec 254 nsec +99 nsec

Maybe we should remove UP027 and implement a reverse version of it.

@carljm
Copy link
Contributor

carljm commented Aug 9, 2024

I agree that the current UP027 doesn't make any sense. Tbh I'm not a big fan of the rule in either direction; the perf difference is not massive and won't matter for most code (and may change in future as faster-cpython team have plans to improve generator perf), and I don't think there's a strong stylistic argument in either direction either. But if it makes sense to have the rule at all, it would make sense to have it in the other direction (to prefer list comprehension.)

@MichaReiser
Copy link
Member

@AlexWaygood should we deprecate this rule as part of 0.6?

@Avasam
Copy link

Avasam commented Aug 12, 2024

On the note of an inverse rule, I think that's basically what I'm asking in #11839 ?

@MichaReiser MichaReiser added this to the v0.6 milestone Aug 12, 2024
@MichaReiser MichaReiser self-assigned this Aug 12, 2024
@AlexWaygood
Copy link
Member

@AlexWaygood should we deprecate this rule as part of 0.6?

Yeah, makes sense to me. I'd hold off with implementing a reverse rule, though; I agree with @carljm that there doesn't seem to be a strong argument for preferring either

@MichaReiser
Copy link
Member

There are cases where pure generators are faster, but only when they're large enough which is way beyond the size someone would unpack ;)

ruff on  ruff-0.6 [$!] is 📦 v0.5.7 via 🐍 v3.12.4 via 🦀 v1.80.0 took 2s 
❯ python -m timeit "a = sum([i for i in range(1_000_000)])"
10 loops, best of 5: 31.3 msec per loop

ruff on  ruff-0.6 [$!] is 📦 v0.5.7 via 🐍 v3.12.4 via 🦀 v1.80.0 took 2s 
❯ python -m timeit "a = sum((i for i in range(1_000_000)))"
10 loops, best of 5: 27 msec per loop

❯ python -m timeit "a = sum((i for i in range(10_000_000)))"
1 loop, best of 5: 288 msec per loop

ruff on  ruff-0.6 [$!] is 📦 v0.5.7 via 🐍 v3.12.4 via 🦀 v1.80.0 
❯ python -m timeit "a = sum([i for i in range(10_000_000)])"
1 loop, best of 5: 357 msec per loop

@MichaReiser MichaReiser removed the needs-decision Awaiting a decision from a maintainer label Aug 12, 2024
MichaReiser added a commit that referenced this issue Aug 12, 2024
Co-authored-by: Alex Waygood <[email protected]>
Closes #12754
AlexWaygood pushed a commit that referenced this issue Aug 14, 2024
Co-authored-by: Alex Waygood <[email protected]>
Closes #12754
MichaReiser added a commit that referenced this issue Aug 14, 2024
Co-authored-by: Alex Waygood <[email protected]>
Closes #12754
MichaReiser added a commit that referenced this issue Aug 14, 2024
Co-authored-by: Alex Waygood <[email protected]>
Closes #12754
tdesveaux added a commit to tdesveaux/buildbot that referenced this issue Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants