MAINT Black formatting prework #20260

thomasjpfan · 2021-06-14T13:46:29Z

Reference Issues/PRs

Related to #18948

What does this implement/fix? Explain your changes.

This PR adds the documentation and prepares the CI to check black's formatting. This PR also fixes the black config to run correctly with black . I think we can do this all on the main branch:

Merge this PR.
Merge PR that runs black formatting. This PR also turns on CI to check black formatting.
Merge PR to add a .git-blame-ignore-revs that ignores the black formatting commit.
Cherrypick this PR onto 0.24.x to update docs.

Any other comments?

I experimented with resolving merge conflicts merging black formatted code into some PRs. I think they are not difficult to resolve and should not be a major blocker.

CC @rth @NicolasHug

rth

Very minor comments, otherwise looks good!

Thanks Thomas!

Edit: CI needs investigating though.

rth · 2021-06-14T13:53:42Z

.pre-commit-config.yaml

+    rev: 21.6b0
+    hooks:
+    - id: black
+      language_version: python3


Are you sure this is necessary?

You mean the python3 config or the black pre-commit config as whole?

As far as I know the precommits will not be enabled as long as the user does install the optional pre-commit developer dependency.

pre-commit support is not strictly necessary, so we can remove.

I don't think I have an opinion on this. Any option would do.

rth · 2021-06-14T13:54:05Z

azure-pipelines.yml

@@ -45,8 +45,12 @@ jobs:
        versionSpec: '3.9'
    - bash: |
        # Include pytest compatibility with mypy
-        pip install pytest flake8 mypy==0.782
+        pip install pytest flake8 mypy==0.782 black


Suggested change

pip install pytest flake8 mypy==0.782 black

pip install pytest flake8 mypy==0.782 black==21.6b0

If we pin a specific version of black in the CI, we should also pin it in doc/developers/contributing.rst.

However I am not sure it's worth the added complexity. I am pretty sure that devs will often have the latest version of black in their dev env.

I would be in favor of trying without pinning. If it proves annoying we can always decide to pin a specific version later until the black format spec is declared final.

After having used black on some other projects for a while, I find that that if one doesn't pin black version one will get in trouble eventually. For instance my current scikit-learn dev env is from 2020 and the version there produces a slightly different formatting to version 21.6b0 . Similarly there might a change in the next release that is not be fully compatible etc. So it's not like flake8 where the version doesn't matter.

And for sure the pre-commit version of black and the CI version need to match exactly. Otherwise we will get CI failure for each new black release.

Alright, I am fine with pinning, then (if others agree). The documentation needs to be updated accordingly.

rth · 2021-06-14T13:55:09Z

doc/developers/contributing.rst

+
+  .. prompt:: bash $
+
+      git config blame.ignoreRevsFile .git-blame-ignore-revs


Is there a way to make it enabled by default? In particular for the online git blame UI of github.com?

I read the link. Unfortunately it's not possible at this time:

The one caveat is that GitHub and GitLab do not yet support ignoring revisions using their native UI of blame. So blame information will be cluttered with a reformatting commit on those platforms. (If you’d like this feature, there’s an open issue for GitLab and please let GitHub know!)

The request for github to support blame ignore files is: https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256 So using git blame with the github UI will require one more click to get pass the black commit. On the other hand, editors support blame.ignoreRevsFile pretty well.

I "hearted" the feature request on github.community.

rth · 2021-06-14T14:09:33Z

It might be good to also have instructions of what to do if an existing PR has conflicts due to black. As in run black before merging master in (not after). And because it's temporary, it might be easier to have it as an issue or a discussion entry rather than documentation section. The main part is that we need to be able to link to it, which is currently not possible with documentation paragraphs.

Documentation CI has some issues apparently.

rth · 2021-06-14T14:16:42Z

Opened #20261

NicolasHug · 2021-06-14T14:18:33Z

Maybe a sticky issue will be more visible? This is probably where contributors will post to ask for help rather than in dicussions

rth · 2021-06-14T14:21:23Z

As you prefer. It's just looked like a perfect use case for a Discussion Q&A and I'm trying to try using it somewhere :) I'm fine with an issue as well, feel free to re-open one otherwise.

It's possible to pin a discussion entry as well.

thomasjpfan · 2021-06-14T14:28:21Z

The merge conflicts you get from merging with the black commit has the same workflow as we have now. What kind of additional information would you like to see in this pinned issue?

rth · 2021-06-14T14:31:32Z

I meant to indicate that users should definitely not try manually resolving merge conflicts they get before running black. And if they have run,

git merge upstream/main

already, they can cancel it with git merge --abort, run black then re-run the merge.

NicolasHug · 2021-06-14T14:33:08Z

Also the "run black" part of the instruction is probably not just black . since users don't necessarily have https://github.com/scikit-learn/scikit-learn/pull/19031/files merged already, where all the black config is.

thomasjpfan · 2021-06-14T14:40:48Z

Also the "run black" part of the instruction is probably not just black . since users don't necessarily have https://github.com/scikit-learn/scikit-learn/pull/19031/files merged already, where all the black config is.

For current PRs, I think the simplest workflow is: Fetch and merge with upstream/main then run black .

rth · 2021-06-14T14:42:06Z

Aww, so that works? I though it would create lots of merge conflicts. But maybe I'm mistaken. Anyway I guess we will find out after we apply black on main )

NicolasHug · 2021-06-14T14:43:43Z

For current PRs, I think the simplest workflow is: Fetch and merge with upstream/main then run black .

upstream/main is black-formatted but the PR isn't. Wouldn't git pull upstream main create tons of conflicts?

thomasjpfan · 2021-06-14T14:47:17Z

Aww, so that works? I though it would create lots of merge conflicts. But maybe I'm mistaken. Anyway I guess we will find out after we apply black on main )

I'll experiment a little more with some more PRs. I compared the conflicts I get merging pre-black and post-black and the complexity didn't change.

NicolasHug · 2021-06-14T14:47:36Z

yeah:

(sklearn-dev) ➜  scikit-learn git:(main) ✗ git co -b black
Switched to a new branch 'black'
(sklearn-dev) ➜  scikit-learn git:(black) ✗ black sklearn/pipeline.py
reformatted sklearn/pipeline.py
All done! ✨ 🍰 ✨
1 file reformatted.
(sklearn-dev) ➜  scikit-learn git:(black) ✗ git ci -am "formatted pipeline"
[black de95fdf1d] formatted pipeline
 1 file changed, 129 insertions(+), 122 deletions(-)
(sklearn-dev) ➜  scikit-learn git:(black) ✗ code .
(sklearn-dev) ➜  scikit-learn git:(black) ✗ git co main
Switched to branch 'main'
Your branch is ahead of 'nicolashug/main' by 162 commits.
  (use "git push" to publish your local commits)
(sklearn-dev) ➜  scikit-learn git:(main) ✗ git co -b my_pr
Switched to a new branch 'my_pr'
(sklearn-dev) ➜  scikit-learn git:(my_pr) ✗ git diff
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: sklearn/pipeline.py
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ sklearn/pipeline.py:463 @ class Pipeline(_BaseComposition):
        fit_params_last_step = fit_params_steps[self.steps[-1][0]]
        with _print_elapsed_time('Pipeline',
                                 self._log_message(len(self.steps) - 1)):
-            y_pred = self.steps[-1][1].fit_predict(Xt, y,
+            y_pred = self.steps[-1][1].fit_predict(Xt, y,  # some comment
                                                    **fit_params_last_step)
        return y_pred

(sklearn-dev) ➜  scikit-learn git:(my_pr) ✗ git ci -am "minor comment"
[my_pr 5b8564afc] minor comment
 1 file changed, 1 insertion(+), 1 deletion(-)
(sklearn-dev) ➜  scikit-learn git:(my_pr) ✗ git merge black
Auto-merging sklearn/pipeline.py
CONFLICT (content): Merge conflict in sklearn/pipeline.py
Automatic merge failed; fix conflicts and then commit the result.

TLDR: even a one-line change will create conflicts and we can expect those absolutely everywhere

thomasjpfan · 2021-06-14T14:54:30Z

TLDR: even a one-line change will create conflicts and we can expect those absolutely everywhere

I see what you mean. Would the pinned issue be advice on how to resolve merge conflicts?

NicolasHug · 2021-06-14T14:59:17Z

Would the pinned issue be advice on how to resolve merge conflicts?

More like on how to avoid them, but yes. It will probably look like

git merge --abort (depending on whether they're in the middle of conflicts)
install black
run black . --include all the stuff to inlcude -- exclude all the stuff to exclude aka all the stuff that's in https://github.com/scikit-learn/scikit-learn/pull/19031/files and that we can't assume to be in the PR author's history
git pull upstream main

ogrisel

LGTM assuming we can come with a pinned issue to help contributors resolve the conflicts in their PR with a few lines to copy/paste as @NicolasHug suggested.

thomasjpfan · 2021-06-14T21:01:14Z

I went through a few PRs and I think the best workflow for resolving black related merge conflicts is:

Abort your merge: git merge --abort
git fetch upstream
Merge your PR with the commit right before the black formatting commit.

git merge COMMIT_BEFORE_BLACK

This will import the black config and deal with existing merge conflicts before black formatting.

Install and run black .
Commit black formatted changes.
Merge with main.

ogrisel · 2021-06-15T08:20:53Z

What is nice with this approach described in #20260 (comment) is that the commit right before black will have the necessary black configuration, so no need to pass complex command line options.

rth · 2021-06-17T11:44:02Z

Should we merge? I'll add the above answer once we have the hash of the last commit before black was applied.

ogrisel · 2021-06-17T15:03:52Z

Should we merge? I'll add the above answer once we have the hash of the last commit before black was applied.

We should probably not merge #20260 if #18948 is not ready to merge just after it (which is not the case at the moment).

But +1 for merging #20260 and #18948 once the latter has green CI.

ogrisel · 2021-06-18T16:01:36Z

We still need to create a pinned issue with the instructions of #20260 (comment) right?

rth · 2021-06-18T16:03:31Z

Yes, I think.

ogrisel · 2021-06-18T16:12:46Z

I opened: #20301 and pinned it. Feel free to improve it directly @thomasjpfan @rth @NicolasHug or others.

thomasjpfan added 4 commits June 13, 2021 14:58

CI Use black in pre-commit hook

c872acd

DOC Adds documentation for black

6af962f

CI Run black on azure

7ebae0f

MAINT Fix black config

f07eb5e

rth approved these changes Jun 14, 2021

View reviewed changes

thomasjpfan mentioned this pull request Jun 14, 2021

MNT Applies black formatting to most of the code base #18948

Merged

thomasjpfan added 2 commits June 14, 2021 10:42

BLD Fix doc build

6d7bdf2

REV Removes black pre-commit

5697d2c

CLN Pins black

9f873b2

ogrisel approved these changes Jun 14, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into pre_black_work

7ade7d6

rth mentioned this pull request Jun 14, 2021

Auto-format Python code emscripten-core/emscripten#14449

Open

thomasjpfan added 3 commits June 17, 2021 13:07

MAINT Adds asv_benchmarks/env

9b2ae4e

Merge remote-tracking branch 'upstream/main' into pre_black_work

2cf265a

Merge remote-tracking branch 'upstream/main' into pre_black_work

196511d

thomasjpfan merged commit 0e7761c into scikit-learn:main Jun 17, 2021

This was referenced Jun 17, 2021

DOC Adds black information to contribution guide in stable #20291

Merged

MAINT Adds target_version to black config #20293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT Black formatting prework #20260

MAINT Black formatting prework #20260

	pip install pytest flake8 mypy==0.782 black
	pip install pytest flake8 mypy==0.782 black==21.6b0


		.. prompt:: bash $

		git config blame.ignoreRevsFile .git-blame-ignore-revs

MAINT Black formatting prework #20260

MAINT Black formatting prework #20260

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment