Use ruff format instead of black

[yaugenst-flex]

Closes #1662

I tried to keep these changes as minimal as possible so that we don't re-format the whole repo in a minor release. Following changes:

• Downgrade ruff 0.3.2 -> 0.2.2, as this is what most closely matches our black version (23.12.1). ruff >0.3 already partly implements the 2024 code style.
• Explicitly exclude docs/faq and docs/notebooks, as these seem to be implicitly ignored currently (see flexcompute/tidy3d-notebooks#6). We should work toward not excluding them, but not for 2.7 I guess.
• There are some known deviations from black, but these are quite minimal. In total, 6 files in tidy3d/ were changed from the previous formatting. There are two changes in line breaks, the rest are just trailing commas.
• We still depend on black for its Python API in make_script.py, and I didn't want to remove this for 2.7.
• Changed config to fix = false by default, as we set --fix in the pre-commit hook, and lint fixing during development should be left to the developer's IDE config imo
• Changed target-version from py310 to py39 as that is the minimal Python version that we support. The linting should not target a newer Python version as that can lead to errors. It was also misconfigured (and ignored) previously, as it somehow ended up in the bumpmyversion config.
• Split the ruff section in pyproject.toml up into [tool.ruff] and [tool.ruff.lint].

Overall I think this simplifies the workflow a bit and we also get a nice 4x speedup:

Down the line it would be nice to update to a later version of ruff, but that will involve adopting at least the 2024 code style, which will reformat the repo quite a bit, so this should probably be done in a major release.

@tylerflex I think the best way to handle pytest fixtures being detected as missing imports is to just be explicit about it, i.e. using # noqa: F401. This issue has been raised on both black and ruff before and the consensus seems to be that no one wants to implement pytest-specific logic for this case. The alternative would be to exclude F401 module- or even linter-wide, but I think that's probably a bad idea.

@momchil-flex Could you perhaps check if these changes are compatible with the backend?

[daquinteroflex]

Hi Yannick, thanks for this! Everything you're suggesting sounds great, and thanks for finding out these bugs.

I guess we'd have to check/sync this with the backend too as they are using black to format - maybe we can migrate it to ruff. There was some chat about doing this between releases too, ultimately it'd be nice to just have the PRs with all the lint changes on both repos ready to merge after a corresponding release. Tyler can say more but maybe the next major release might not be imminent exactly so wonder if we want to apply a solution sooner.

[momchil-flex]

I think this should be fine separately from the backend (we can migrate that to ruff too whenever we want) right @daquinteroflex? I mean, on the backend we only black specific folders, not the submodules.

[tylerflex]

@yaugenst-flex do we want to just use ruff 0.3? I dont think we're tied to a specific black version, it's just a bit annoying to upgrade black as it affects all of the code. but maybe we should just use the most modern ruff?

[tylerflex]

Explicitly exclude docs/faq and docs/notebooks, as these seem to be implicitly ignored currently (see https://github.com/flexcompute/tidy3d-notebooks/issues/6). We should work toward not excluding them, but not for 2.7 I guess.

Nice 👍

We still depend on black for its Python API in make_script.py, and I didn't want to remove this for 2.7.

Is there a way to replicate this using ruff? Also, I'm not sure if anyone uses this tool to be honest.

Changed config to fix = false by default, as we set --fix in the pre-commit hook, and lint fixing during development should be left to the developer's IDE config imo

Changed target-version from py310 to py39 as that is the minimal Python version that we support. The linting should not target a newer Python version as that can lead to errors. It was also misconfigured (and ignored) previously, as it somehow ended up in the bumpmyversion config.

Split the ruff section in pyproject.toml up into [tool.ruff] and [tool.ruff.lint].

All three sound good to me

[yaugenst-flex]

@yaugenst-flex do we want to just use ruff 0.3? I dont think we're tied to a specific black version, it's just a bit annoying to upgrade black as it affects all of the code. but maybe we should just use the most modern ruff?

i'm all for it, i was just a bit scared of reformatting pretty much the whole codebase. if we're not too worried about that i'll just switch to the latest ruff (0.4.3)

Is there a way to replicate this using ruff? Also, I'm not sure if anyone uses this tool to be honest.

not directly (since ruff has no python api), but i think i can come up with something

[momchil-flex]

The biggest issue with reformatting the whole codebase is if it will make currently open PRs annoying to rebase. In principle it seems like it is not too bad, but we've been postponing linting all of the backend code until 2.7 is merged into develop (e.g. in preparation for the official release). Maybe best to do that then, too?

[yaugenst-flex]

Ok yeah that makes sense, let's defer it until then. This PR is pretty much ready to go, I'll rebase and update ruff once everything else is done.

[tylerflex]

ok. BTW I'll tag this as 2.7 & .0 in an attempt to keep track of when things should be merged.

[daquinteroflex]

I think this should be fine separately from the backend (we can migrate that to ruff too whenever we want) right @daquinteroflex? I mean, on the backend we only black specific folders, not the submodules.

Yeah, that's true! I guess was specifically thinking mainly in terms of avoiding conflicting pyproject.toml-defined versions in the same venv environment which can occur in the poetry migration.We can add this linting upgrade so that it only uses ruff also in that PR and that way we're consolidated there too

[momchil-flex]

I mean, it kinda sounds like we may keep our life simple if we either switch both already, or wait until after the backend poetry refactor. I don't really see a big need to swtich so I'm fine either way (or even switching the frontend only for now if you don't think it will create headaches)

[daquinteroflex]

Yeah, makes sense to do it separately sooner. I'll modify the existing linting PR on the backend to this version so we can merge it to the current backend whenever this front-end one is merged too.

[momchil-flex]

Ok, yeah, let's do it after merging pre/2.7 to develop

[tylerflex]

Just a note:

• It would be nice if we didnt need to # noqa: F401 the log_capture or other fixture imports
• and also if we didnt need to # noqa: F811 when these fixtures are passed into tests.

[yaugenst-flex]

I think the only way to do that is if we use a custom lint config for the tests (which is fine and maybe we should do that anyway). This easy to add. What do you think?

[tylerflex]

I think the only way to do that is if we use a custom lint config for the tests (which is fine and maybe we should do that anyway). This easy to add. What do you think?

I think it's worth it if it's not too painful. Cuts down on the amount of random failing tests we're going to have to deal with in the future

[tylerflex]

@yaugenst-flex @daquinteroflex when is the best time to merge this? should we do it soon-ish?

[momchil-flex]

After we merge 2.7 branches to develop - which I'm not opposed to doing soon. But it may be simpler to first merge the last remaining big things, adjoint, eme, subpixelspec.

[daquinteroflex]

On the solver side, I am not super sure if we want to do it at the same time? That PR is set up already and just need to be updated with the latest changed

[daquinteroflex]

Also @yaugenst-flex some people use the scripts/test_local.sh which we probably need to decide it's future with this accordingly. Personally, I'd prefer if we all used the pre-commit

[yaugenst-flex]

@daquinteroflex yeah let's keep those kinds of changes for another PR, this one is getting a bit out of hand already...

[tylerflex]

@yaugenst-flex When this is merged, could you update the notion page for developer guide so people know the new workflow? (for example, it still mentions black)

Thanks

https://www.notion.so/flexcompute/Tidy3D-Developer-Guide-23ceee49660e42fca06484bfcaa96b5c

[momchil-flex]

Also @yaugenst-flex some people use the scripts/test_local.sh which we probably need to decide it's future with this accordingly. Personally, I'd prefer if we all used the pre-commit

Wait so why not just modify the scripts/test_local.sh script to use ruff instead of black though?

[yaugenst-flex]

Wait so why not just modify the scripts/test_local.sh script to use ruff instead of black though?

ah, that's an oversight.. i'll fix it. i agree with @daquinteroflex though that this should all go through the pre-commit, ~or at least via poetry run ...~ (nevermind, can just poetry run test_local.sh)

[daquinteroflex]

So just to update on the order, this needs to be merged in order to relink the submodule on the backend to pre/2.7 btw. But the backend PR is updated up to only missing this.

[yaugenst-flex]

This is ready to go. I updated ruff now too and fixed a few more lint (and actual) errors that cropped up due to the update, tests are passing now. If you could have another quick look I think we can squash and merge @daquinteroflex @momchil-flex

[momchil-flex]

I'm running backend tests just in case will merge tomorrow if all good https://github.com/flexcompute/tidy3d-core/actions/runs/9389775774

[daquinteroflex]

I had a dinner a bit earlier, so I've now updated the submodule to this latest state, and rerun ruff 0.4.8 on the backend. Running backend tests in this redeployed action as a result, also a deployment which I'll test the sims tomorrow morning.

Will have a proper glance rather than a quick look at this PR too in the morning.

[weiliangjin2021]

Now running test_local.py doesn't format the code, but just print the diff: "Would fix 1 error"?

[momchil-flex]

Hm yeah on the backend it still just formats directly, I think we should remove the --check, any thoughts @yaugenst-flex @daquinteroflex ?

[yaugenst-flex]

Sure we can do that. Probably the thought was that a "test" shouldn't be changing things.