10000 How to fix merge conflicts related to the black code style reformatting of the scikit-learn code base · Issue #20301 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
ogrisel opened this issue Jun 18, 2021 · 16 comments
Closed

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

ogrisel opened this issue Jun 18, 2021 · 16 comments

Comments

@ogrisel
Copy link
Member
ogrisel commented Jun 18, 2021 •
edited by adrinjalali
Loading

In #18948 we recently started to use black to format almost all of scikit-learn's code, so it's possible that your PR is conflicting with the main branch due to the new code formatting style. Here are some instructions to resolve conflict that you may encounter when merging your PR's branch with the main branch.

Here we assume that upstream is an alias for the https://github.com/scikit-learn/scikit-learn git repo or its ssh variant. You can confirm that on your end by checking the output of git remote -v.

You could try to directly merge with main and then apply black locally, but this is likely to introduce a lot of conflicts due to the formatting style change. Instead, we recommend the following workflow:

  • Abort any previous merge attempts in your branch: git merge --abort. If you weren't in the middle of a merge before this will fail with "fatal: There is no merge to abort". It's normal, you can safely ignore this message.

  • Fetch all the commits from the main repo git fetch upstream

  • Merge your PR branch with the commit right before the black formatting commit:

    git merge 0e7761cdc4f244adb4803f1a97f0a9fe4b365a99

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

  • Cherry-pick the commit the updates the black config with target_version:

    git cherry-pick e8f58cddb55777e0e40195aaf64d5a890b9fdaac

Then, locally apply black to all the code:

  • pip install black==21.6b0

  • To format the code changes in your PR branch from the top level scikit-learn source folder:

    git diff $(git merge-base upstream/main HEAD) --name-only -- '*.py' | xargs black

  • Commit black formatted changes.

  git commit -am "apply black"
  • Merge your branch with main: 
    git fetch upstream main
git merge upstream/main

This should work without code-style related conflicts.

If git blame is important to you

For git blame to work locally run:

git config blame.ignoreRevsFile .git-blame-ignore-revs
@ogrisel ogrisel mentioned this issue Jun 18, 2021
Merged
@ogrisel ogrisel pinned this issue Jun 18, 2021
@ogrisel ogrisel changed the title How to fix conflicts related to the black code style reformatting of the scikit-learn code base How to fix merge conflicts related to the black code style reformatting of the scikit-learn code base Jun 18, 2021
This was referenced Jun 18, 2021
Merged
Merged
@thomasjpfan
Copy link
Member

I updated commit to cherry-pick e8f58cd which adds the target_version into the black config.

@thomasjpfan
Copy link
Member

I just tried this approach with one of my older PRs. I think straight merging is easier. Running black yourself with the commit right before black introduces its own merge conflicts.

@thomasjpfan
Copy link
Member
thomasjpfan commented Jun 19, 2021 •
edited
Loading

The best tool for the job looks to be: https://github.com/akaihola/darker :

git merge --abort
git fetch upstream main

# Update black config and resolve conflicts before black
git merge 0e7761cdc4f244adb4803f1a97f0a9fe4b365a99
git cherry-pick e8f58cddb55777e0e40195aaf64d5a890b9fdaac

# There is a bug in darker where isort is a hard dependency and fix will be in the next release.
# isort will **not** run when calling `darker`
pip install isort darker black==21.6b0

darker --revision upstream/main .

git commit -m "STY Apply black only on diff"
git merge upstream/main

Edit Found a better workflow with:

I think I got a workflow that works with. This only runs black on the changed files, which actually uses target_version.

git diff $(git merge-base upstream/main HEAD) --name-only -- '*.py' | xargs black

instead of running black . The original message was updated with this instruction.

@thomasjpfan thomasjpfan reopened this Jun 19, 2021
@jjerphan jjerphan mentioned this issue Jun 21, 2021
Open
@alfaro96
Copy link
Member

Should we pin this issue for future contributors?

@ogrisel
Copy link
Member Author
ogrisel commented Jun 21, 2021 •
edited
Loading

Should we pin this issue for future contributors?

It is pinned.

@ogrisel
Copy link
Member Author
ogrisel commented Jun 21, 2021 •
edited
Loading

git diff $(git merge-base origin/main HEAD) --name-only -- '*.py' | xargs black

The problem is that it's not trivial to get this command to work on windows: it will only work in a git bash or a WSL command-line prompt. In particular it will not work in anaconda prompt and I am not sure if it works to activate a conda env in a git bash prompt to be able to use black there.

I am not even sure for the use of xargs in git bash.

@ogrisel
Copy link
Member Author
ogrisel commented Jun 21, 2021 •
edited
Loading

Maybe it's easier to reformat only the files changed in the PR with black by passing all the necessary config and filenames as command line argument, commit the result and then merge the latest main?

@jjerphan jjerphan mentioned this issue Jun 21, 2021
Open
@thomasjpfan
Copy link
Member

Maybe it's easier to reformat only the files changed in the PR with black by passing all the necessary config and filenames as command line argument, commit the result and then merge the latest main?

That would work. Is there a nice way to get all the files that the PR changed in windows? Maybe just copying and pasting is good enough?

  1. git merge-base upstream/main HEAD
  2. git diff COMMIT_FROM_STEP_1 --name-only -- "*.py"
  3. black -t py39 --exclude ... FILES_FROM_STEP_2

@helper-uttam
Copy link
Contributor

hey can someone help me?
I had opened a PR in scikit-learn scorer.py, but it is not getting merged :(
I need to know that if I had done anything wrong? or I should contribute in some other way? :(

@rth
Copy link
Member
rth commented Jun 21, 2021

Will answer in #20316

@jjerphan jjerphan mentioned this issue Jun 23, 2021
Closed
hongshaoyang added a commit to hongshaoyang/scikit-learn that referenced this issue Jun 23, 2021
@hongshaoyang
Black formatting
98c0c12 
Following
scikit-learn#20301 (comment)
@adrinjalali
Copy link
Member

Could we also add the git ignore command thingy here? We should have one place where all the required changes and configurations in one place. I'm quite confused now.

@thomasjpfan
Copy link
Member

Could we also add the git ignore command thingy here?

I added a small section for git blame to work locally run. i.e:

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

@adrinjalali
Copy link
Member

Thanks @thomasjpfan , I updated its title, otherwise 👍

@thomasjpfan
Copy link
Member
thomasjpfan commented Jun 27, 2021 •
edited
Loading

Here is a possible simplified workflow: Edit: Maybe not great as well.

  1. git fetch upstream main
  2. Install black and my fork of darker:
pip install black==21.6b0 \
git+https://github.com/thomasjpfan/darker@custom_sklearn_darker
  1. darker --revision upstream/main . -v
  2. Commit black formatted code
  3. git merge upstream/main

My fork hard codes the configuration we are using, which means a contributor does not need to update pyproject.toml. Since I am hard coding the configuration, I think it would be good to wait on if we want #20412 .

Edit

I have been updating some of my older PRs and found that this works with the least conflicts:

  1. git fetch upstream main
  2. git merge upstream/main
  3. Fix conflicts and git commit -n (ignoring pre-commit hooks)
  4. black .
  5. Commit black formatted code.

@tupui tupui mentioned this issue Jul 1, 2021
Closed
8 tasks
@AchyuthGowthamK
Copy link

Merge conflicts can occur when you try to merge code that has been modified in multiple branches or by multiple developers. If the changes are conflicting, it can be difficult to resolve the conflict and merge the code successfully.

If you are experiencing merge conflicts related to the black code style reformatting of the scikit-learn codebase, here are some steps you can follow to try to resolve the conflict:

  1. First, make sure you have the latest version of the codebase and any relevant branches.
  2. Identify the files that have merge conflicts by running git status or looking for files with the unmerged status.
  3. Open the files with conflicts in a text editor and look for the <<<<<<< and >>>>>>> markers that indicate the start and end of a conflict.
  4. Decide which changes you want to keep and which you want to discard. You can do this by comparing the changes and deciding which version is more important or relevant.
    5.Once you have decided which changes to keep, delete the markers and the changes you do not want to keep. Save the file.
    6.Repeat this process for all the files with conflicts.
    7.Once you have resolved all the conflicts, stage the changes by running git add ..
    8.Commit the changes with a message explaining what you did to resolve the conflicts, using git commit -m "Resolved merge conflicts related to black reformatting".
    9.Push the changes to the relevant branch with git push.

Keep in mind that these are just general guidelines, and the specific steps you need to take to resolve merge conflicts may vary depending on the nature of the conflicts and the changes you are trying to merge. If you are having trouble resolving the conflicts, you may want to seek assistance from other developers or from the scikit-learn community.

@kungfuchicken kungfuchicken mentioned this issue Dec 20, 2022
Closed
@njzjz njzjz mentioned this issue Jan 27, 2023
Merged
@piercefreeman piercefreeman mentioned this issue May 19, 2023
Merged
@jjerphan jjerphan mentioned this issue Jun 21, 2023
Merged
@The-Compiler The-Compiler mentioned this issue Jul 25, 2023
Open
@jjerphan
Copy link
Member

I think we can close this PR now that two years have elapsed. What do you think?

@betatim betatim closed this as completed Nov 1, 2023
@jjerphan jjerphan unpinned this issue Nov 28, 2023
@freddyheppell freddyheppell mentioned this issue Jun 6, 2024
Merged
4 tasks
@adrinjalali adrinjalali mentioned this issue Feb 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@ogrisel @rth @betatim @adrinjalali @thomasjpfan @jjerphan @alfaro96 @helper-uttam @AchyuthGowthamK
0