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

thomasjpfan · 2020-12-01T03:51:43Z

Reference Issues/PRs

Related to #11336

What does this implement/fix? Explain your changes.

This runs black on most of the codebase and fixes the flake8 issues as well.
The configuration for flake8 is in setup.cfg.
The configuration for black is in pyproject.toml.
Here is a reference on how we would setup git blame with black. It requires merging this PR and then having another PR that adds a .git-blame-ignore-revs. I would say that git blame is an advanced feature of git, which is often used to track down the PR that created the diff.
Makes the max-line-length 88, which is the default of black.

Any other comments?

For reference, django created a enhancement proposal for why they introduced using black for formatting.

rth

Thanks @thomasjpfan !

Here is a reference on how we would setup git blame with black.

Aww that's interesting, it's unfortunate that Github still doesn't support that.

A few comments below. Could you please check that we are running black on the externals dir?
Also please add it to the .pre-commit-config.yaml

-   repo: https://github.com/psf/black
    rev: 19.3b0
    hooks:
    -   id: black
        types: [file, python]

That's the recommended way of not forgetting to apply black on each commit..

rth · 2020-12-01T07:20:45Z

pyproject.toml

+      \.eggs         # exclude a few common directories in the
+    | \.git          # root of the project
+    | \.mypy_cache
+    | examples


If we use black for code, it might make sense to use it for examples as well maybe? Because if we don't use consistent formatting for all python it can be confusing for contributors.

In yesterday's meeting we discussed that applying black to examples could potentially lead to big /scary code sections. No strong opinion on my side, although it's true that we've been following slightly different conventions for examples. Typically in the notebook-style ones, we write the import at the beginning of each cell instead of at the top of the file.

For me it comes down to a UX thing. When reading an example and the code block uses up a significant portion of the vertical space, does this make the example harder to read?

rth · 2020-12-01T07:21:26Z

pyproject.toml

+    | build
+    | dist
+    | doc
+    | build_tools


Are there python files in there, that we should avoid to format?

rth · 2020-12-01T07:22:08Z

pyproject.toml

+    | examples
+    | build
+    | dist
+    | doc


The config.py can be formatted at least, I think.

rth · 2020-12-01T07:22:59Z

setup.cfg

+    dist,
+    examples,
+    sklearn/externals,
+    doc,


Docs and examples should still be linted by flake8 I think.

rth · 2020-12-01T08:43:51Z

Do we plan to then apply black automatically on PRs or do we ask contributors to do that manually?

NicolasHug · 2020-12-01T09:18:21Z

Thanks @thomasjpfan

Other stuff we'll need to do ~~reasonably fast after this is merged~~ (EDIT: I think we should actually do them before):

have a black CI check
update our contributing guidelines to indicate to run black. Also mention the pre-commit hook as a possibility (but not a requirement)
make a pinned issue (sort of like Lots of new untracked files after merging with master? #17341) with instructions for PR authors on how to apply black and then solve potential merge conflicts.

Should we be doing all this (including this PR) in another branch, and merge into master in one shot?

Do we plan to then apply black automatically on PRs or do we ask contributors to do that manually?

Thing is, applying black automatically to PRs will likely force contributors to git pull and git merge each time. If they're not tech savy enough to read our docs and apply black themselves, they'll likely be confused by that process as well.

Also @thomasjpfan do you know if we can use black on Cython files as well?

thomasjpfan · 2020-12-04T22:03:30Z

Should we be doing all this (including this PR) in another branch, and merge into master in one shot?

Given the size of this PR, I wouldn't want to do anymore than apply black in this PR. I can get those PRs which can quickly be merged after this one.

Thing is, applying black automatically to PRs will likely force contributors to git pull and git merge each time. If they're not tech savy enough to read our docs and apply black themselves, they'll likely be confused by that process as well.

I think the best way to do this is with a script they can use locally. This script would:

Make sure the local repo is clean.
Fetch upstream/master
Cherry pick the pyproject.toml and setup.cfg from upstream/master
Run black and commit.
Try to merge with upstream/master.
Ask for help if there is an issue with the merging. I think 90% of the time the merge will be not too bad.

Also @thomasjpfan do you know if we can use black on Cython files as well?

Not supported yet: psf/black#359

NicolasHug · 2020-12-05T09:23:47Z

Given the size of this PR, I wouldn't want to do anymore than apply black in this PR.

I agree. I'm suggesting to merge this PR and the other black-related ones into a new branch rather than on master. This way, we can then merge all black-related changes in one shot into master, and avoid any asynchronicity.

I think the best way to do this is with a script they can use locally. This script would...

This is quite complex and we'd need to support 2 of these (unix and windows). It's probably simpler for now to have a black CI and to document how to apply black locally, mentioning commit hooks as an option.

glemaitre · 2020-12-16T09:15:35Z

Regarding the timing, I think that just after the release of 0.24 looks like a good idea.

Regarding the pre-commit, we could use a documentation likewise pandas: https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#python-pep8-black

It shows how to do it manually and refers to pre-commit if people want to. Since we have a section pre-commit, we can add additional stuff there.
Pandas as a CI for the style as well.

NicolasHug · 2020-12-16T09:23:00Z

No opinion from my side but if we do that after 0.24, we'll need to make sure the stable docs contain the black-related info, which probably means cherry-picking the relevant PRs and pushing a new doc release.

Regardless, I still think we should do all this in a separate branch. And this PR should actually be the last one to be merged: it will introduce tons of merge conflicts and we need to point contributors to docs indicating how to apply black. So we should focus on the docs PRs first.

glemaitre · 2020-12-16T09:37:32Z

No opinion from my side but if we do that after 0.24, we'll need to make sure the stable docs contain the black-related info, which probably means cherry-picking the relevant PRs and pushing a new doc release.

Good point. It could avoid updating the documentation for that matter.

Regardless, I still think we should do all this in a separate branch. And this PR should actually be the last one to be merged: it will introduce tons of merge conflicts and we need to point contributors to docs indicating how to apply black. So we should focus on the docs PRs first.

Looks like a good plan. I can create a PR and we can always change the targeted branch at the last minute.

thomasjpfan · 2020-12-17T20:40:50Z

I opened #19031 to apply style changes that would make it so this PR can just be "apply black". I think this would simplify things because this PR would just be for applying black.

Regardless, I still think we should do all this in a separate branch. And this PR should actually be the last one to be merged: it will introduce tons of merge conflicts and we need to point contributors to docs indicating how to apply black. So we should focus on the docs PRs first.

I think I am missing something. What kind of work do you see us doing on this separate branch?

NicolasHug · 2020-12-18T08:37:21Z

I think I am missing something. What kind of work do you see us doing on this separate branch?

Basically what I listed in #18948 (comment). I find it safer and simpler to have everything ready in one branch and just merge the whole branch in one shot than do incremental changes. When this PR is merged, we need to have the docs already, the CI check, and ideally a sticky issue pointing to conflict resolution instructions. Otherwise, current PR authors will be lost in what to do.

rth · 2021-06-12T13:17:53Z

Any way we could move forward on this PR? #19031 is waiting for a second review.

It would be good to have documentation on how to deal with existing PRs, but just a pinned issue could do too. If contributors encounter difficulties in the PRs they will say so/open issues and we can move forward from there. Which is still better than being stuck here discussing how many PRs we need for this :)

NicolasHug · 2021-06-12T13:53:43Z

A pinned issue could do without the docs, as long as this situation doesn't last too long. That is, we should release soon, with up-to-date docs. We will need to have these docs at some point anyway, so I don't see why we can't write them now.

At the very very least we'll need a CI check, otherwise we'll have to submit re-formatting PRs every now and then and this will keep creating more conflicts.

We can do all that here, or in a separate branch as suggested above, which would be IMHO much smoother

thomasjpfan · 2021-06-12T16:14:20Z

Was there a discussion on moving to the 88 line length limit somewhere? (it is black's default)

I'll begin work on the separate branch this weekend (now 😅).

NicolasHug · 2021-06-12T18:01:43Z

Was there a discussion on moving to the 88 line length limit somewhere? (it is black's default)

Yes, but I think people ended up being fine with it. IMHO using black with a smaller line length would cause even more vertical space being used, so I would stick to the default

thomasjpfan · 2021-06-14T14:00:55Z

I updated this PR to build on top of #20260

This PR does two things:

Enables black check in CI.
Runs black .

thomasjpfan · 2021-06-14T18:39:50Z

BTW @thomasjpfan maybe we should have used extend-ignore in flake8 config instead of re-listing the default exceptions?

I prefer the way we have it now because it allows us to write the human readable for each ignored violation. Secondly, the flake8 default may change with different versions of flake8. By explicitly setting them, we have more control over ignored violations.

rth

Assuming the only commit here is applying black + enabling CI once #20260 is merged LTGM.

ogrisel

Same comment as @rth. @thomasjpfan feel free to update the PR and merge them in the right order.

ogrisel · 2021-06-17T18:39:21Z

Thanks @thomasjpfan!

rth · 2021-06-17T18:47:03Z

Glad that this happened!

ensure SVDD passes numpydoc validation (scikit-learn#20463) check for svdd in `test_sparse.py:check_svm_model_equal` to avoid calling `.predict_proba`

rth reviewed Dec 1, 2020

View reviewed changes

thomasjpfan force-pushed the black_formating branch 2 times, most recently from 66be950 to 5ea7d64 Compare December 13, 2020 01:38

glemaitre mentioned this pull request Dec 16, 2020

ENH add fontname argument in export_graphviz for non-English characters #18959

Merged

thomasjpfan mentioned this pull request Dec 17, 2020

MNT Adds black config and fixes formatting before black #19031

Merged

Base automatically changed from master to main January 22, 2021 10:53

lorentzenchr added this to the 1.0 milestone Mar 19, 2021

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

thomasjpfan mentioned this pull request Jun 14, 2021

MAINT Black formatting prework #20260

Merged

thomasjpfan force-pushed the black_formating branch 2 times, most recently from 160f7c8 to 1db386c Compare June 14, 2021 14:00

thomasjpfan added the No Changelog Needed label Jun 14, 2021

BLD Fix doc build

6d7bdf2

thomasjpfan mentioned this pull request Jun 14, 2021

STY Minor style changes to prepare for black #20265

Merged

thomasjpfan force-pushed the black_formating branch from 285f2d6 to bc2d15d Compare June 15, 2021 13:40

rth approved these changes Jun 17, 2021

View reviewed changes

ogrisel approved these changes Jun 17, 2021

View reviewed changes

MAINT Adds asv_benchmarks/env

9b2ae4e

thomasjpfan force-pushed the black_formating branch from bc2d15d to b9478e0 Compare June 17, 2021 17:09

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

2cf265a

thomasjpfan mentioned this pull request Jun 17, 2021

STY Minor fix for black formatting #20289

Merged

thomasjpfan added 2 commits June 17, 2021 13:19

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

196511d

STY Run black

bea7802

thomasjpfan force-pushed the black_formating branch from b9478e0 to bea7802 Compare June 17, 2021 17:21

thomasjpfan added 2 commits June 17, 2021 13:42

CI Run black check on azure

ace973a

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

e4b5606

thomasjpfan merged commit 82df489 into scikit-learn:main Jun 17, 2021

rth mentioned this pull request Jun 17, 2021

Fix flake8 in examples #20296

Closed

ogrisel mentioned this pull request Jun 18, 2021

How to fix merge conflicts related to the black code style reformatting of the scikit-learn code base #20301

Closed

lorentzenchr mentioned this pull request Jun 25, 2021

Black introduced a lot of interrupted strings #20365

Closed

tupui mentioned this pull request Jul 1, 2021

MAINT/STY: use Black formatting scipy/scipy#14330

Closed

8 tasks

ilayn mentioned this pull request Jul 6, 2021

STY: Maths formatting scipy/scipy#14354

Closed

QuentinSoubeyran mentioned this pull request Oct 5, 2021

Move CI to GitHub Actions scikit-optimize/scikit-optimize#1074

Open

njzjz mentioned this pull request Jan 27, 2023

add and run pre-commit to format codes deepmodeling/deepmd-kit#2279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

thomasjpfan commented Dec 1, 2020 •

edited

Loading

rth left a comment

rth Dec 1, 2020

NicolasHug Dec 1, 2020

thomasjpfan Dec 4, 2020

rth Dec 1, 2020

rth Dec 1, 2020

rth Dec 1, 2020

rth commented Dec 1, 2020

NicolasHug commented Dec 1, 2020 •

edited

Loading

thomasjpfan commented Dec 4, 2020

NicolasHug commented Dec 5, 2020

glemaitre commented Dec 16, 2020

NicolasHug commented Dec 16, 2020

glemaitre commented Dec 16, 2020

thomasjpfan commented Dec 17, 2020

NicolasHug commented Dec 18, 2020

rth commented Jun 12, 2021

NicolasHug commented Jun 12, 2021

thomasjpfan commented Jun 12, 2021

NicolasHug commented Jun 12, 2021

thomasjpfan commented Jun 14, 2021

thomasjpfan commented Jun 14, 2021

rth left a comment

ogrisel left a comment

ogrisel commented Jun 17, 2021

rth commented Jun 17, 2021

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

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

Conversation

thomasjpfan commented Dec 1, 2020 • edited Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

rth left a comment

Choose a reason for hiding this comment

rth Dec 1, 2020

Choose a reason for hiding this comment

NicolasHug Dec 1, 2020

Choose a reason for hiding this comment

thomasjpfan Dec 4, 2020

Choose a reason for hiding this comment

rth Dec 1, 2020

Choose a reason for hiding this comment

rth Dec 1, 2020

Choose a reason for hiding this comment

rth Dec 1, 2020

Choose a reason for hiding this comment

rth commented Dec 1, 2020

NicolasHug commented Dec 1, 2020 • edited Loading

thomasjpfan commented Dec 4, 2020

NicolasHug commented Dec 5, 2020

glemaitre commented Dec 16, 2020

NicolasHug commented Dec 16, 2020

glemaitre commented Dec 16, 2020

thomasjpfan commented Dec 17, 2020

NicolasHug commented Dec 18, 2020

rth commented Jun 12, 2021

NicolasHug commented Jun 12, 2021

thomasjpfan commented Jun 12, 2021

NicolasHug commented Jun 12, 2021

thomasjpfan commented Jun 14, 2021

thomasjpfan commented Jun 14, 2021

rth left a comment

Choose a reason for hiding this comment

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel commented Jun 17, 2021

rth commented Jun 17, 2021

thomasjpfan commented Dec 1, 2020 •

edited

Loading

NicolasHug commented Dec 1, 2020 •

edited

Loading