CI: Ruff

[ax3l]

Ruff is an extremely fast Python linter and code formatter. It replaces tools such as autoflake, black, flake8, pyflakes, or pylint.

Close #4366

We already use ruff in ImpactX and pyAMReX, this adds it to WarpX using the same fast check & Python code formatter methods.

Reviewer note: This pull request is best reviewed commit-by-commit.

• merge Python: Errors & Linting #5124 first, then rebase
• address additional issues

[dpgrote]

I was looking over the changes in the python scripts and see a some places where the changes will break things. I'll add comments for those that I see. But this looks great otherwise!

[dpgrote]

If I were formatting this, I would do this, which I think is easier to read (maybe I'm just more used to it)

parser.add_argument("--identifier",
                    help = "Unique identifier for finding compilation line in the log file",
                    default = "WarpX/Source")

The arguments aligned with the routine name, and a space before and after the =.

[ax3l]

Thanks. I think the reason that this is broken to newlines in black is that it will reduce the diff size when adding more or removing arguments in a later PR, which makes code easier to review (less files changed).

This is auto-formatted using black style (FAQ), as we do in all other projects already:
https://docs.astral.sh/ruff/formatter/

Developers can install a pre-commit:

python -m pip install pre-commit
pre-commit install

to get auto-formatting, otherwise the PR auto-bot does it for them.

[dpgrote]

My own preference is allowing longer lines. Keeping this as one line is more succinct and easier to read (for me at least).

[ax3l]

We usually do not mingle with the default, which are currently around 100 characters (soft limitish with some heuristics). The bot auto-breaks them. This makes diffs on github easier to read.

We can increase that limit manually, but since a bot formats them we did keep the defaults on all other BLAST projects so far.

Looking at line 38, the limit is still pretty long, so I would not bump it further.

[dpgrote]

I like this formatting better. Is there an option with ruff to use this style?

[ax3l]

Nope, it just uses black style. As far as I read the style is intentional, the extra break reduces the amount of lines changed when adding/removing new arguments in PRs to dictionaries. (Note also the trailing ,.)

Black aims for consistency, generality, readability and reducing git diffs.

The prior formatting would often change the first and last line on updates to the dict.

[dpgrote]

There's probably not an option for this, but I much prefer no spaces around * (and /) since it makes it look more like standard math expressions, with the grouping of multiplied terms. Compare these two expressions

x = (3 + 2 * a) * (b * c + 55 * d)
x = (3 + 2*a)*(b*c + 55*d)

Which is easier to understand?

[ax3l]

Spaces around operators is a pretty standard thing these days in C++ and Python. PEP8 requires it even.

For long and block formulas, we can always disable formatting, so we can have nice alignments when we need it, e.g.,

# fmt: off
x = a * ( 42 * b
         + 3 * c
         - 7 * d
        ) 
# fmt: on

[dpgrote]

Oh yes, I'm well aware that the spaces around operators is standard, but I never liked it for * and /. I've never understood why something that is so much harder to read and more prone to typos would be the standard.
BTW, PEP8 actually says to use white space around the lower priority operators, so my example would be

x = (3 + 2*a) * (b*c + 55*d)

similar to what I would do.

[ax3l]

I know 😅 That is how I type as well but I don't mind the auto-formatter adding spaces by now anymore :D

In the end, there are more precedences than I have spaces for, e.g., C++:
https://en.cppreference.com/w/cpp/language/operator_precedence

[dpgrote]

"I don't mind the auto-formatter adding spaces" meaning you've gotten used to the bad formatting decision :)

[dpgrote]

I prefer spaces around = since it makes the text less dense and easier to read.

[ax3l]

I think PEP8 requires for arguments no spaces and spaces only for assignments. Independent of PEP8, black style seems to do the same.

[dpgrote]

PEP8 does say no space around = for arguments, but only shows this when the arguments are all on one line. This is fine. But for multiline function calls, I find it much more readable to have spaces around =. It is inconsistent to require spaces around all operators (to spread things out) except in this one case where it would in fact make the code easier to read.

[ax3l]

You are right, I think it's a pragmatic choice in black. They do spaces for assignments and assignments in definitions (default values) but not in call site arguments. I kinda like it because I like my call sites compact, but as all style choices it might come down to taste and what one is used to :)

[roelof-groenewald]

It's a little unfortunate that the standard is inconsistent with powers:

(xi - xi_0) ** 2

but

c**2

Is there a way to have this be consistent, i.e., use ()**2 instead of () ** 2?

[ax3l]

I think here are the rules on powers: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-breaks-binary-operators

[roelof-groenewald]

This is an awesome change to enforce consistency in Python code! 🎉

I left two comments on things I think would be nice to change / add (formatting of powers in arithmetic expressions and forcing f-strings). But I'll approve this PR so long and those can be changed in future PRs.
If you agree, @ax3l, it would be nice to undo the changes to the capacitive_discharge analysis files before merging.

[jwestern]

I just want to raise awareness that style changes can have costs for some users, which might not be obvious. Particularly users working with custom extensions of WarpX. This commit created (a lot of) trivial auto-merge conflicts in our workflow, which took a while to track down, and I'm still looking for a workaround.

[ax3l]

Hi @jwestern,

Thank you for raising this.

Fully understand. This style change is intended to be a one-time change, which is once a bit painful (e.g., forks and open PRs) but will actually make all follow-up work easier to maintain, review, pull and merge - if, you also use ruff in your PRs (automatic) and forks (please set up). Sorry for this one-time change, we try to keep those overall reformats to a minimum.

As a general recommendation for extending WarpX, we do move fast indeed. As people in other projects (e.g., the Cling people on LLVM), maintaining a true fork can be challenging contrary to extending a project.
This is why, we recommend to do the following if you like to extend WarpX:

• plan which parts are generally needed/useful or enabling for your workflow and contribute them upstream
• add additional callbacks/hooks and options you might need upstream here, and only
• extend WarpX via our Python bindings (can still couple in more C++, e.g., via pybind11)
  • you can also use WarpX and extend it as a pure C++ library, in fact both our app executable and Python bindings link against exactly a libwarpx, but it is more productive to go via Python as glue code

That way, you would not have to maintain a full fork and only would have to update breaking API changes, which change less often.

Examples of a fork of WarpX that has same challenge as you: https://github.com/AMReX-Microelectronics/artemis

Examples of companies extending WarpX and contributing common/core functionality: Modern Electron (a few years back), TAE, Helion, etc.