airbyte-ci: format python using ruff instead of isort+black

[postamar]

This picks up where #29853 stalled. At the time, we didn't have airbyte-ci format. I re-used @aaronsteers 's pyproject.toml settings here. The resulting diff is surprisingly not huge.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Dec 13, 2023 6:20pm

[bnchrch]

Similar to before. No objections to Ruff.

Its faster, and lets us move from 2 tools to 1.

The fact that the rules are slightly different is annoying but formatting is to enforce a consistent style. The details of the style we choose are not important.

However, Given that @alafanechere is actively working in the area I would defer to him for final approval

[postamar]

Thanks for the review! Yes, I'll wait until his pre-commit hook + workflow changes have merged.

[aaronsteers]

@bnchrch, @postamar

I have a bit of good news and bad news. It looks like between the prior PR and this one, ruff announced and released ruff format as a replacement for black. @bnchrch - you actually were the one who made me aware this was even in the works with the prior PR. So, that's the good news: ruff can now do everything black could do, and (apart from mypy), ruff now replaces "all the things".

Bad news: this PR uses ruff format (the black replacement) but not also ruff check --fix. That full changeset has over 1K files changed and is here:

• Python auto-fix from ruff check --fix #33269

Worse news: some rules from ruff format and ruff check --fix and incompatible, so running those in sequence will lead to an infinite chain of modified files. Since black rules were always the "opinionated" and "correct" way to format files, the format rules should win when in conflict with check --fix, and indeed, the blog post tells us some rules we need to disable in order to make them compatible.

So - we have a choice in front of us:

• We can merge this as-is, and take check --fix as a follow-on PR.
• We can do all in one go, but we should at minimum disable conflicting rules between format and check.

PS - I'm a bit bummed that ruff check --fix and ruff format are different operations, but I guess I can see the logic from their side.

[bnchrch]

Very excited to see that they expanded.

@aaronsteers I can't seem to find any reference to where ruff check --fix and ruff format conflict.

I could find a list of deviations in behaviour from black but so far it seems like if we are running ruff's version of linting and formatting we should be safe.

But! I imagine im just being blind in my reading on Friday.

I did run a little test in this PR
#33275

Where I had the following order of operations

1. ruff format
2. commited
3. run check --fix
4. commited
5. ruff format
6. commited
7. ruff check --fix

And these two things seem to be true

1. ruff check --fix changes are not formatted properly
2. ruff format changes are correct according to ruff check

This means during fix if we ever run ruff check --fix we must run ruff format directly after.

and during check we should ensure we run ruff check before ruff format --check

But I think at this point I would be for us adding the formatting code in one PR and the check code in another. Since we werent doing much linting to begin with and I would love to keep the momentum going and get ruff into our code base. Because youre right, it is fast.

[aaronsteers]

@bnchrch :

Here's the link for documented conflicts: https://docs.astral.sh/ruff/formatter/#format-suppression

Also: here's the commit from format after check --fix: 71ac7b1

After a quick manual review, some of the updates from this commit seem to be driven by what could be a difference in line length preferences. Might be worth more research to see if an inconsistent line length in my process caused some of those additional diffs.

[maxi297]

This will generate mypy error as # type: ignore[override] needs to be applied to the line where stream: "AbstractFileBasedStream" is

[postamar]

Thanks! I'll check the other annotations touched by this diff as well.

[postamar]

The others are OK, I checked with mypy using the config at airbyte-cdk/python/mypy.ini

[postamar]

@aaronsteers somehow I knew it was too good to be true. Still, this change in this PR seems like it's not only safe but also desirable, considering that ruff format is like black but better.

There's nothing stopping anyone from adding check --fix and its ruleset in an incremental fashion. It seems like a nontrivial amount of work, though.

[alafanechere]

I'm in for using ruff in place of black and isort but I'd love to make sure this tool change is not changing our formatting rules. Ideally this tool change should not require a new formatting of the repo.
I belive there might be some configuration tweaking to get this result.

[postamar]

That's legitimate. I don't have the time nor the interest in getting to understand the ruff rules, so I'll let someone else take over from here.

[aaronsteers]

@postamar re:

@aaronsteers somehow I knew it was too good to be true. Still, this change in this PR seems like it's not only safe but also desirable, considering that ruff format is like black but better.

There's nothing stopping anyone from adding check --fix and its ruleset in an incremental fashion. It seems like a nontrivial amount of work, though.

Yes, 100% agreed. Let's go forward with this as a first step. And incrementally, we can add additional format/lint autofixes in a future iteration.

@alafanechere - can you say more about your point above? The changes auto-applied here are basically bringing our code to meet black auto format standards, but using Ruff which is faster and better long-term. I didn't think we had any similar auto formatting for Python code before this, but I could be mistaken. (Or maybe we have auto formatting for some directories but not others?)

[aaronsteers]

Looks great. 👍 Approving for my part. ✅

As discussed in the threads above, we can always do a follow-up to add more linting and autofixes.

One thing that might be nice is (maybe under "contributing" and maybe in a follow-up pr?) a section on "code style" or python "dev standards" - with a mention of what auto-formatter we use and how developers/contributors can invoke it manually. (Just a thought... please disregard if this already exists.)

[girarda]

The change looks good but you might have to approve and merge despite the failing cdk mypy check unless you plan on resolving the typing errors too. The check runs on all modified cdk files so a lot of failures are surfaced on this PR

[postamar]

@alafanechere I looked into it some more and no, we can't minimize the diff more by tweaking the config. The short of it is, there is no config, both black and ruff are opinionated formatters. Ruff has somewhat different opinions on certain matters, which explains a lot of the diff: https://docs.astral.sh/ruff/formatter/black/ . Note that black's style is also continuously evolving, see https://black.readthedocs.io/en/stable/the_black_code_style/future_style.html . I admittedly don't have a horse in this race but it all strikes me as quite benign.

Given these facts, I'm hoping that you might change your mind. Otherwise the logical alternative is that we not just keep using black, but this exact version of black, which doesn't strike me as better.

[natikgadzhi]

Yolo, one tool is better than four, we should switch and clean out our documentation. It's going to be easier on us, and it's going to be easier on external contributors and new engineers starting on the team.

[natikgadzhi]

Just continuing dicussion here, obviously we would need to reformat everything.

Here's what I think we should do:

1. Replace Black + isort with Ruff:

• Merge Python Ruff support (config only) #29866
• Then, replace black and isort in airbyte-ci format check and format fix. To clarify, we will use ruff format, not ruff check. Why? Well, because the lift to get check to pass is big.
• Push up formatting fixes after running ruff format locally, this will get all python code in compliance.

2. Remove black and isort from all packages dependencies. Nice side-effect: we're using 2 years old version of black (22.*) and it has security holes in it, so — we'd get rid of those.
3. Remove black and isort from all internal Python packages, including airbyte-ci and the CDK.
4. In CDK specifically, remove flake8. Instead, replace check with ruff check. This is going to hurt, I suppose.
5. While we're at it, we should make sure our CDK and airbyte-ci mypy configuration targets Python 3.10+ and make any neccesary fixes.