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

Dev/black #3840

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Dev/black #3840

merged 5 commits into from
Jul 27, 2023

Conversation

lillekemiker
Copy link
Contributor

@lillekemiker lillekemiker commented Jul 19, 2023

What type of PR is this? (check all applicable)

  • Refactor
  • Feature (dev feature and reformatting)
  • Bug Fix
  • Optimization
  • Documentation Update

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because:

Description

Introducing black to the code base as a first step towards this:
https://github.com/invoke-ai/InvokeAI/discussions/3721

Related Tickets & Documents

  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No : Not applicable

[optional] Are there any post deployment tasks we need to perform?

All active branches will be affected by this and will need to be updated.

This PR adds a new github workflow for black as well as config for pre-commit hooks to those who wish to use it

@psychedelicious
Copy link
Collaborator

Thanks for working on the actions.

We have some large PRs and branches pending and need to get to a point where we can freeze PRs, merge all the big kahunas, and only then can we do a massive formatting sweep. We've been waiting for months to do this but there was way too much changing to break history with a bit format PR.

I believe we settled on base black config but with line length of 100 or 120.

@lillekemiker
Copy link
Contributor Author

@psychedelicious No worries, I totally get that. I personally find it harder to expand on existing files right now because I have to turn off my auto formaters, but that’s me. Just say the word when you want to pull the trigger. Git makes it look harder than it really is to make this branch mergable. Basically it’s as easy as copying all the files from main and running black on them. No need to actually work out merge conflicts.

@psychedelicious
Copy link
Collaborator

I hear you, I disabled my autoformat on save too 😮‍💨 the concern was more that with a big formatting commit, we create a boundary that makes diffs hard to reason about, and the code was in very high flux. Getting close now!

@lillekemiker
Copy link
Contributor Author

Is there a reason you want to do 100-120 character lines? I always liked the standard 88 (leaving an 8 character buffer from the classic 80).

@psychedelicious
Copy link
Collaborator

Our monitors are generally bigger than punchcards or hardware terminals now (excellent talk btw), though of course it's just a preference.

The frontend code will be getting the 100 or 120 width treatment soon too

@lillekemiker
Copy link
Contributor Author

Yeah, I totally get the issue with one big commit that reformats the whole code base and creates a diff wall. If you want, I’ll happily break it up into individual files if that helps. Especially if we can identify files that are not being changed right now. I just didn’t want to spam you with PR’s without first discussing.

@psychedelicious
Copy link
Collaborator

I believe we are pretty close to enabling the formatting but I'll request @ebr and @brandonrising for their opinions

@lstein
Copy link
Collaborator

lstein commented Jul 20, 2023

Yeah, I totally get the issue with one big commit that reformats the whole code base and creates a diff wall. If you want, I’ll happily break it up into individual files if that helps. Especially if we can identify files that are not being changed right now. I just didn’t want to spam you with PR’s without first discussing.

I actually did the "reformat everything with black" thing last winter during the 2.3 release and one of the gotchas was that "git blame" ended up blaming me for every line of code in the repository. After some searching I found a technique that preserves blame across the black boundary, applied it, and it improved the situation. Do you know about it? If not, I'll try to retrace my steps.

@lstein
Copy link
Collaborator

lstein commented Jul 20, 2023

Is there a reason you want to do 100-120 character lines? I always liked the standard 88 (leaving an 8 character buffer from the classic 80).

I prefer the longer lines. 100 is pretty comfortable for me.

Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great but I'd like to wait on this until the pace of change slows down. About a week, I'd say.

@lillekemiker
Copy link
Contributor Author

@lstein there’s something here about how to get around the git blame issue:
https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html

I’m not entirely clear on how it works based on this link alone, but maybe one big squash merge and then add the commit hash to ‘.git-blame-ignore-revs’?
Does that look like what you did?

@mikeage
Copy link

mikeage commented Jul 21, 2023

@lstein , I think you were looking for this: #2855

(note that if you do squash merges, instead of merge commits, you'll need to do a new PR to add the squashed commit of this PR)

pip install .[test]

# - run: isort --check-only .
- run: black --check .
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're using pre-commit, may I recommend the following action instead:

      - uses: pre-commit/action@v3.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool. I did not know this existed. Personally I'm a little torn on using it because there's not much gained by using it and it leaves us dependent on an externally managed action. There's also a slight difference in what is being run in the precommit hooks right now. For the sake of speed, they only run black on what is being committed, while the current GHA is running black on everything.
If others prefer using this action, though, I don't feel super strongly about it.

Copy link

@mikeage mikeage Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a dev here, so please remember that I cannot speak for the project at all :-). I understand what you're saying about external actions, although it is pretty official. Perhaps at least run pre-commit run -a instead of black directly? It reduces the chance that at some point someone might want to change a parameter or something and will need to remember to sync in both places.

IIRC, I think the action it can also be configured to add comments with places that were missed if you pass a token (but possibly not on a PR from a fork). It also includes a git diff after it runs, so instead of just a binary "failed", you can see what it fixed, which can be helpful for new contributors who aren't aware of the coding conventions.

@lillekemiker lillekemiker force-pushed the dev/black branch 4 times, most recently from 8d0f5ca to 7880318 Compare July 22, 2023 20:09
@zopieux
Copy link
Contributor

zopieux commented Jul 23, 2023

My 2 cents:

  • I agree that using and enforcing a formatter/linter is great practice, in general but especially so for OSS development accepting external contributors. It is very cumbersome having to deal with each project's individual coding style. This should be taken care of by computers, not busy humans – I couldn't care less whether this repo is using 80, 88 or 120 char lines. A nicely formatted code base feels way more welcoming to contributions. I'm currently refraining from non-trivial contributions to the backend, even though I would like to, because of the current situation. I cannot be bothered to disable code formatters in 2023, they're integral to my workflow. Big 👍 to the intent behind this PR.
  • I wasn't aware of the blame-skip trick. This is awesome, but if it's too impractical still, note that an alternative to the massive whole-repo reformat is to use a formatter that is able to only apply formatting to git-modified lines. This way, historical code will slowly converge to being formatted correctly as actual logic changes are contributed. Sadly PSF black does not yet support this.

@lillekemiker
Copy link
Contributor Author

@zopieux - Actually by running black with the pre-commit hooks only, black would only be applied to the files changed in the given commit.
However, I personally think that doing it a little bit at a time would make things even less transpararent. Consider a PR that makes a 1 line change to a 500 line file, except black changes all 500 lines. That gets confusing quickly. And with blame you can always go back in time more than one change, so if you see blame pointing to a commit called "apply black", you just hit the button one more time.

@psychedelicious
Copy link
Collaborator

I'm calling this out on discord to hopefully grab everyone's attention (and any strongly-dissenting opinions).

This will also close #2522.

@lillekemiker lillekemiker force-pushed the dev/black branch 2 times, most recently from 080e252 to f24f053 Compare July 26, 2023 01:17
Copy link
Member

@hipsterusername hipsterusername left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this to 120 characters? If we can, I think there is no reason not to get this merged in today @ebr @brandonrising

@lstein
Copy link
Collaborator

lstein commented Jul 27, 2023 via email

Copy link
Member

@ebr ebr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lillekemiker could I ask you to please rebase on current state of main, apply the 120 char line length change, and force-push one more time? I will make sure this is reviewed today and will try to merge ASAP.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@lstein
Copy link
Collaborator

lstein commented Jul 27, 2023 via email

@ebr
Copy link
Member

ebr commented Jul 27, 2023

black is opinionated (for better or worse), so the line length is the most significant configuration option.

Any in-flight PRs would have to apply black --line-length 120 .

There are also a couple of other options, e.g. --skip-magic-trailing-comma ("Don't use trailing commas as a reason to split lines"), but I'm not sure it's worth our time tweaking that right now.

Re: conflict resolution - @lillekemiker just force-pushed the latest changes, so I will make sure to review it ASAP so we can merge it before any more conflicts are introduced. I will add a commit with the blame-skip trick.

@lillekemiker
Copy link
Contributor Author

@lillekemiker could I ask you to please rebase on current state of main, apply the 120 char line length change, and force-push one more time? I will make sure this is reviewed today and will try to merge ASAP.

Done!

@ebr
Copy link
Member

ebr commented Jul 27, 2023

@lillekemiker: did you use the black --fast option to get baseinvocation.py, commands.py and compel.py to reformat? without --fast, it fails to parse the AST. And/or, which version of black do you have installed?
we also need it added to dev dependencies (I can do that)

Nevermind - not an issue with the latest black

.github/workflows/style-checks.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@ebr
Copy link
Member

ebr commented Jul 27, 2023

@lillekemiker did you have to pass --include installer/lib/* ? these files are currently being ignored by black due to lib/ present in .gitignore. Seems like we should be able to remove that without consequence, but i'm wondering how you got Black to format these files in the first place.

@lillekemiker
Copy link
Contributor Author

@lillekemiker did you have to pass --include installer/lib/* ? these files are currently being ignored by black due to lib/ present in .gitignore. Seems like we should be able to remove that without consequence, but i'm wondering how you got Black to format these files in the first place.

I found that if I run black using pre-commit run -a, then it formats everything. I'm guessing either git or pre-commit does the file fetching instead of black when done that way.

Copy link
Member

@ebr ebr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

🚨 I will add added another commit into .git-blame-ignore-revs before merging. Suggest we merge this as a rebase rather than a squash or merge commit, such that the SHA of the black commit remains unchanged.

@lstein - any current in-flight PRs will need to pip install black followed by black --line-length 120 . (also @Millu please take note). But I don't think we can avoid conflict resolution entirely.

Please approve, and we'll merge if this is acceptable.

also remove lib/ from gitignore as it is hiding the installer code
from 'black'
@mikeage
Copy link

mikeage commented Jul 27, 2023

. Suggest we merge this as a rebase rather than a squash or merge commit, such that the SHA of the black commit remains unchanged.
.

Both a rebase and a squash will change the SHA. Only a merge commit, or another PR with the SHA of the squash or rebased commit.

@lstein lstein self-requested a review July 27, 2023 16:44
Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ebr
Copy link
Member

ebr commented Jul 27, 2023

. Suggest we merge this as a rebase rather than a squash or merge commit, such that the SHA of the black commit remains unchanged.
.

Both a rebase and a squash will change the SHA. Only a merge commit, or another PR with the SHA of the squash or rebased commit.

right, good point. Let's try a merge commit, though it means the blacked commit won't be on main. Worst case, we'll add another commit with the correct SHA

@hipsterusername hipsterusername merged commit 7d458eb into invoke-ai:main Jul 27, 2023
7 of 8 checks passed
@zopieux
Copy link
Contributor

zopieux commented Jul 27, 2023

Let's go! Can finally hack on that codebase :-)

@lillekemiker lillekemiker deleted the dev/black branch July 27, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants