Ruff linter #586

juanitorduz · 2024-01-11T20:46:04Z

Continuation of an apparently stale PR #539

juanitorduz · 2024-01-11T21:26:22Z

I made similar changes as in pymc-devs/pymc#7091 .

Let's have an initial feedback round 😄 !

codecov-commenter · 2024-01-11T22:21:57Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c5b96d9) 80.92% compared to head (be86730) 80.92%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   80.92%   80.92%   -0.01%     
==========================================
  Files         162      162              
  Lines       46641    46627      -14     
  Branches    11399    11399              
==========================================
- Hits        37746    37732      -14     
  Misses       6667     6667              
  Partials     2228     2228

Files	Coverage Δ
pytensor/compile/debugmode.py	`61.46% <100.00%> (ø)`
pytensor/graph/basic.py	`89.08% <100.00%> (ø)`
pytensor/graph/destroyhandler.py	`68.98% <100.00%> (ø)`
pytensor/graph/fg.py	`89.08% <100.00%> (-0.04%)`	⬇️
pytensor/graph/rewriting/basic.py	`73.04% <100.00%> (-0.02%)`	⬇️
pytensor/link/c/basic.py	`87.61% <100.00%> (ø)`
pytensor/link/c/cmodule.py	`55.74% <ø> (ø)`
pytensor/link/c/params_type.py	`80.41% <ø> (ø)`
pytensor/link/c/type.py	`75.98% <100.00%> (ø)`
pytensor/link/jax/dispatch/elemwise.py	`81.69% <ø> (ø)`
... and 31 more

... and 1 file with indirect coverage changes

pyproject.toml

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>

juanitorduz · 2024-01-12T19:02:45Z

To fix pyupgrade I had to skip UP031: Use format specifiers instead of percent format because otherwise, we have Found 135 errors which I suggest not to fix on this PR and instead create an issue about it. I do not understand why the previous pre-commit did not catch this 🤔.

maresb

I think this is fine since one rarely overrides builtins, although I'll occasionally do something like

class SpecialString(str):
    """Special instance of str for type hints"""

It may be nice to get another opinion though, e.g. from @ricardoV94.

maresb · 2024-01-13T22:16:10Z

tests/tensor/rewriting/test_basic.py

-                if type(condition[1]) is list:
+                if isinstance(condition[1], list):


Was this change automatic or by hand? I think this is safe, but strictly speaking condition[1] could theoretically be a subclass of list (although subclassing builtin types is generally pretty broken, so nobody should ever do it). The reason I bring it up is because we're a bit stalled on #581 due to similar issues. This PR will make that obsolete, but I'm curious if you have any thoughts.

maresb · 2024-01-13T22:16:22Z

tests/tensor/rewriting/test_basic.py

-                if type(condition[1]) is list:
+                if isinstance(condition[1], list):


Same as above

maresb · 2024-01-13T22:18:13Z

pytensor/link/vm.py

-                assert type(storage_map_list[0]) is list
-                assert type(compute_map_list[0]) is list
+                assert isinstance(storage_map_list[0], list)
+                assert isinstance(compute_map_list[0], list)


Same comment

maresb · 2024-01-13T22:19:04Z

pytensor/scan/op.py

-                    if type(profile) is not bool and profile:
+                    if (not isinstance(profile, bool)) and profile:


maresb · 2024-01-13T22:20:54Z

tests/tensor/rewriting/test_math.py

-                if type(condition[1]) is list:
+                if isinstance(condition[1], list):


All of them were made manually by me as ruff raised an error and suggested this solution 😬.

maresb · 2024-01-14T17:58:19Z

I'm concerned that ruff isn't picking up these pyupgrade fixes: 4c417cc

Note that those were engaged after upgrading pyupgrade from 3.3.1 to 3.15.0.

ricardoV94 · 2024-01-14T18:04:35Z

The isinstance checks seem good, since they are all about built-in types. I'm glad it didn't try to override the type(...) == type(...) with custom types that @maresb mentioned. Is the auto-linter more restricted, or where these changes you did manually @juanitorduz?

Either way, these look okay. Just wanted to know because of the discussion in the other PR

ricardoV94 · 2024-01-14T18:05:50Z

To fix pyupgrade I had to skip UP031: Use format specifiers instead of percent format because otherwise, we have Found 135 errors which I suggest not to fix on this PR and instead create an issue about it. I do not understand why the previous pre-commit did not catch this 🤔.

The precent format is used widely in the C code generation with % locals. That will take some work to refactor so definitely fine to ignore. Would be nice if we could ignore per-file if that's not too many?

juanitorduz · 2024-01-14T18:06:38Z

I changed the defauldict and deque changes manually 🙈. Ruff just raised the errors. What should I fix ?

ricardoV94 · 2024-01-14T18:07:22Z

I changed the defauldict and deque changes manually 🙈. Ruff just gce me the errors. What should I fix ?

Those are fine. The question is whether you changed the type(x) == y to isinstance(x, y) manually?

juanitorduz · 2024-01-14T18:08:23Z

Yes, I did! It was manual work case by case 🙈

ricardoV94 · 2024-01-14T18:10:23Z

I'm concerned that ruff isn't picking up these pyupgrade fixes: 4c417cc

Note that those were engaged after upgrading pyupgrade from 3.3.1 to 3.15.0.

The this is the only thing we should check. Looks good otherwise

juanitorduz · 2024-01-14T18:31:13Z

To fix pyupgrade I had to skip UP031: Use format specifiers instead of percent format because otherwise, we have Found 135 errors which I suggest not to fix on this PR and instead create an issue about it. I do not understand why the previous pre-commit did not catch this 🤔.

The precent format is used widely in the C code generation with % locals. That will take some work to refactor so definitely fine to ignore. Would be nice if we could ignore per-file if that's not too many?

There are many 😄. I would rather create an subsequent issue... What do you think ?

maresb · 2024-01-14T18:53:13Z

This PR already has 70 changed files, and that makes it really difficult to view what's going on with the config.

In an ideal world I think we'd break this up into one PR that does all the formatting changes (assuming they're compatible with both flake8 and ruff). Then there would be a subsequent clean PR that's config-only, but maybe by necessity contains code changes that are not cross-compatible.

That would probably be a bit overkill with too much work, but I do like the idea of smaller PRs.

juanitorduz · 2024-01-14T18:56:08Z

Yeah, this PR escalated super fast 🫠🙈. If you all want we can close this one and start step by step as suggested.

twiecki · 2024-01-15T04:36:09Z

Hm, I'm in favor of sticking with this one. Do we really need to thoroughly go through every line change? I think a spot change is enough, we have unit-tests and a high level of confidence its doing the right thing.

ricardoV94 · 2024-01-15T08:37:54Z

Shall we merge and open issues for the % format and the pyugrade question then?

maresb · 2024-01-15T10:49:04Z

Shall we merge and open issues

Ya, we could do that. The pyupgrade thing makes me a bit nervous like maybe we haven't configured it correctly. But hopefully it's just one rule they haven't implemented.

Let's merge. That'll allow me to move forward on #581 and #488.

ricardoV94 · 2024-01-15T10:49:52Z

Need to sort the conflicts with the PR we just merged!

maresb · 2024-01-15T10:51:52Z

Need to sort the conflicts with the PR we just merged!

Easy, it's just spacing issues. Done.

juanitorduz · 2024-01-15T13:24:35Z

🙌💪🙏

maresb · 2024-01-15T13:29:25Z

I opened #597 for the pyupgrade issue.

Who would like to take care of the % format issue?

juanitorduz · 2024-01-15T13:45:18Z

If there is no expected “deadline” I can take it and work on it bit by bit 🐢

maresb · 2024-01-15T13:48:54Z

Thanks @juanitorduz!!! Indeed this seems pretty low-priority to me. But would you like to open an issue so that we have a record of the todo?

juanitorduz · 2024-01-15T13:50:45Z

I'll do so once I'm on a proper pc (👶 at the moment 😅)

juanitorduz · 2024-01-15T15:53:42Z

Issue created: #601

juanitorduz added 3 commits January 11, 2024 21:32

init

4afa565

isort

8f11ff5

remove autoflake

d433680

juanitorduz marked this pull request as draft January 11, 2024 20:47

juanitorduz added 4 commits January 11, 2024 21:50

formatter

7dc5beb

simplify excludes a bit

507fd25

remove ignores

d982c63

comment about tests ignores

b1c284a

juanitorduz self-assigned this Jan 11, 2024

juanitorduz marked this pull request as ready for review January 11, 2024 21:25

juanitorduz requested review from ricardoV94 and maresb January 11, 2024 21:25

juanitorduz added 2 commits January 11, 2024 22:40

rm setup.cfg

7280917

undo remove isorts

0f37dda

juanitorduz force-pushed the ruff_linter branch from 7f55843 to 0f37dda Compare January 11, 2024 21:58

twiecki approved these changes Jan 12, 2024

View reviewed changes

maresb reviewed Jan 12, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

juanitorduz and others added 2 commits January 12, 2024 08:23

add pyupgrade

bf49e6a

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>

fix pyupgrade

be86730

juanitorduz requested a review from maresb January 12, 2024 19:03

maresb approved these changes Jan 13, 2024

View reviewed changes

maresb mentioned this pull request Jan 14, 2024

Support Python 3.12 #488

Merged

6 tasks

Merge branch 'main' into ruff_linter

4b77c9a

ricardoV94 approved these changes Jan 15, 2024

View reviewed changes

maresb approved these changes Jan 15, 2024

View reviewed changes

ricardoV94 merged commit ccc08ba into pymc-devs:main Jan 15, 2024
52 of 53 checks passed

ricardoV94 mentioned this pull request Jan 15, 2024

Replace pre-commit linters (flake8, isort, black, ...) with ruff #539

Closed

6 tasks

maresb mentioned this pull request Jan 15, 2024

Apply pyupgrade fixes missing from ruff #597

Merged

11 tasks

juanitorduz deleted the ruff_linter branch January 15, 2024 13:23

maresb mentioned this pull request Jan 15, 2024

Update pre-commit hooks #581

Closed

11 tasks

juanitorduz mentioned this pull request Jan 15, 2024

Fix UP031: Use format specifiers instead of percent format in code base #601

Closed

maresb mentioned this pull request Jan 20, 2024

Decide and impose a hard line limit #610

Closed

ricardoV94 added the no releasenotes label Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruff linter #586

Ruff linter #586

juanitorduz commented Jan 11, 2024

juanitorduz commented Jan 11, 2024

codecov-commenter commented Jan 11, 2024 •

edited

juanitorduz commented Jan 12, 2024 •

edited

maresb left a comment

maresb Jan 13, 2024

maresb Jan 13, 2024

maresb Jan 13, 2024

maresb Jan 13, 2024

maresb Jan 13, 2024

juanitorduz Jan 14, 2024

maresb commented Jan 14, 2024

ricardoV94 commented Jan 14, 2024

ricardoV94 commented Jan 14, 2024

juanitorduz commented Jan 14, 2024 •

edited

ricardoV94 commented Jan 14, 2024 •

edited

juanitorduz commented Jan 14, 2024

ricardoV94 commented Jan 14, 2024

juanitorduz commented Jan 14, 2024

maresb commented Jan 14, 2024

juanitorduz commented Jan 14, 2024 •

edited

twiecki commented Jan 15, 2024

ricardoV94 commented Jan 15, 2024

maresb commented Jan 15, 2024

ricardoV94 commented Jan 15, 2024

maresb commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

maresb commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

maresb commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

		if type(condition[1]) is list:
		if isinstance(condition[1], list):

		if type(profile) is not bool and profile:
		if (not isinstance(profile, bool)) and profile:

Ruff linter #586

Ruff linter #586

Conversation

juanitorduz commented Jan 11, 2024

juanitorduz commented Jan 11, 2024

codecov-commenter commented Jan 11, 2024 • edited

Codecov Report

juanitorduz commented Jan 12, 2024 • edited

maresb left a comment

Choose a reason for hiding this comment

maresb Jan 13, 2024

Choose a reason for hiding this comment

maresb Jan 13, 2024

Choose a reason for hiding this comment

maresb Jan 13, 2024

Choose a reason for hiding this comment

maresb Jan 13, 2024

Choose a reason for hiding this comment

maresb Jan 13, 2024

Choose a reason for hiding this comment

juanitorduz Jan 14, 2024

Choose a reason for hiding this comment

maresb commented Jan 14, 2024

ricardoV94 commented Jan 14, 2024

ricardoV94 commented Jan 14, 2024

juanitorduz commented Jan 14, 2024 • edited

ricardoV94 commented Jan 14, 2024 • edited

juanitorduz commented Jan 14, 2024

ricardoV94 commented Jan 14, 2024

juanitorduz commented Jan 14, 2024

maresb commented Jan 14, 2024

juanitorduz commented Jan 14, 2024 • edited

twiecki commented Jan 15, 2024

ricardoV94 commented Jan 15, 2024

maresb commented Jan 15, 2024

ricardoV94 commented Jan 15, 2024

maresb commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

maresb commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

maresb commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

juanitorduz commented Jan 15, 2024

codecov-commenter commented Jan 11, 2024 •

edited

juanitorduz commented Jan 12, 2024 •

edited

juanitorduz commented Jan 14, 2024 •

edited

ricardoV94 commented Jan 14, 2024 •

edited

juanitorduz commented Jan 14, 2024 •

edited