STYLE Reduce merge conflicts with `force_single_line` #50275

MarcoGorelli · 2022-12-15T14:08:53Z

closes STYLE Reduce merge conflicts with force_single_line or force_grid_wrap=1 #50274 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

Exact commands I ran to do this:

 2129  git checkout -b force-single-line
 2130  git fetch upstream
 2131  git reset --hard upstream/main
 2132  sed -i 's/force_grid_wrap = 2/force_single_line = true/g' pyproject.toml 
 2133  pre-commit run isort -a
 2134  git commit -a -m 'force single line = true' --no-verify

I then added a handful of missing noqa: F401 (which you can check in the second commit)

To check that there's nothing unrelated, you could do:

git reset --hard upstream/main
sed -i 's/force_grid_wrap = 2/force_single_line = true/g' pyproject.toml 
pre-commit run isort -a
git remote add marcogorelli [email protected]:MarcoGorelli/pandas.git
git fetch marcogorelli
git diff marcogorelli/force-single-line

and check that the only diff is a handful of noqa: F401 comments and import pandas._testing as tm changes

Performance is unaffected, here some results:

https://www.kaggle.com/code/marcogorelli/asv-pandas-example/notebook?scriptVersionId=113924671

[ 50.00%] · For pandas commit 39ceeb7f <force-single-line> (round 2/2):
[ 50.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[ 75.00%] ··· package.TimeImport.time_import                            697±10ms
[ 75.00%] · For pandas commit 48461699 <main> (round 2/2):
[ 75.00%] ·· Building for conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter...
[ 75.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[100.00%] ··· package.TimeImport.time_import                             679±8ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

[ 50.00%] · For pandas commit 39ceeb7f <force-single-line> (round 2/2):
[ 50.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[ 75.00%] ··· package.TimeImport.time_import                             684±6ms
[ 75.00%] · For pandas commit 48461699 <main> (round 2/2):
[ 75.00%] ·· Building for conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter...
[ 75.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[100.00%] ··· package.TimeImport.time_import                            686±10ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

[ 50.00%] · For pandas commit 39ceeb7f <force-single-line> (round 2/2):
[ 50.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[ 75.00%] ··· package.TimeImport.time_import                             695±9ms
[ 75.00%] · For pandas commit 48461699 <main> (round 2/2):
[ 75.00%] ·· Building for conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter...
[ 75.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[100.00%] ··· package.TimeImport.time_import                            734±20ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

Another result:

[ 50.00%] · For pandas commit 39ceeb7f <force-single-line> (round 2/2):
[ 50.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[ 75.00%] ··· package.TimeImport.time_import                             693±8ms
[ 75.00%] · For pandas commit 48461699 <main> (round 2/2):
[ 75.00%] ·· Building for conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter...
[ 75.00%] ·· Benchmarking conda-py3.8-Cython0.29.32-jinja2-matplotlib-numba-numexpr-numpy-odfpy-openpyxl-pyarrow-pytables-scipy-sqlalchemy-xlrd-xlsxwriter
[100.00%] ··· package.TimeImport.time_import                            662±10ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

In short - it doesn't make a difference to performance

lithomas1 · 2022-12-15T14:40:44Z

We aren't back porting this right?
(Is the plan to wait till no more 1.5.x releases, then merge before 2.0?)

MarcoGorelli · 2022-12-15T14:44:37Z

We aren't back porting this right? (Is the plan to wait till no more 1.5.x releases, then merge before 2.0?)

I think we could just apply the same change to 1.5.x, so:

git checkout -b backport-force-single-line
git fetch upstream
git reset --hard upstream/1.5.x
sed -i 's/force_grid_wrap = 2/force_single_line = true/g' pyproject.toml 
pre-commit run isort -a
git commit -a -m 'force single line = true' --no-verify

Or waiting until there's no more 1.5.x releases would be fine too

phofl · 2022-12-15T16:14:20Z

Agree, don't want to do this on 1.5.x. Lets wait till we are through there

MarcoGorelli · 2022-12-15T16:15:59Z

sure, marking as draft til then

twoertwein · 2022-12-19T21:14:15Z

Would it make sense to replace isort with reorder_python_imports? I believe we would get the same style but benefit from reorder_python_imports's import re-writing.

MarcoGorelli · 2022-12-19T21:21:29Z

reorder-python-imports doesn't support Cython files, so I don't think we could/should use it (yet)

WillAyd · 2022-12-20T22:54:56Z

Does isort allow us to undo this if we ultimately want to revert? +/-0 on this but feels a bit strange since a user wouldn't naturally write or read code this way

github-actions · 2023-01-20T02:10:55Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

MarcoGorelli · 2023-01-20T09:05:12Z

Does isort allow us to undo this if we ultimately want to revert? +/-0 on this but feels a bit strange since a user wouldn't naturally write or read code this way

We could just remove the option, and then it would go back to how it was before

Do users often write or read imports anyway though? I think most contributors just put the import somewhere at the top and then just let isort take care of where and how to place it

MarcoGorelli · 2023-02-15T10:31:18Z

@mroeschke @lithomas1 thoughts on getting this in before the RC?

Exact commands I ran to do this:

git fetch upstream && git reset --hard upstream/main && sed -i 's/force_grid_wrap = 2/force_single_line = true/g' pyproject.toml && pre-commit run isort --all-files; git commit -a -m 'force single line = true' --no-verify (this is the first commit)
pre-commit run ruff --all-files (I then manually fixed up a handful of missing noqas - this is the second commit)
git ls-files | xargs sed -i 's/from pandas import _testing as tm/import pandas._testing as tm/g' (this is the third commit)

The third commit was to fix this unwanted pattern

pandas/.pre-commit-config.yaml

Lines 269 to 271 in fe85cbf

    
                       # imports from pandas._testing instead of `import pandas._testing as tm` 
        
                       |from\ pandas\._testing\ import 
        
                       |from\ pandas\ import\ _testing\ as\ tm

which, given the current import style, wasn't actually being caught most of the time

mroeschke · 2023-02-15T18:27:55Z

IMO I don't think this should go into the RC yet. Since this is a style change that touches a lot of files, I think there should be a few more positive responses before committing to this.

simonjayhawkins · 2023-02-22T15:19:42Z

The goal here is to remove merge conflicts (in the longer term).

Of course while there are backport branches active with these changes this would increase the probability of a merge conflict.

So probably best to do it now (and also apply the change script to 2.0.x now separately) since perhaps it should have been done before the rc.

If we need to do another 1.5.x release then we would maybe have a few merge conflicts to resolve.

I am generally +1 on this change.

MarcoGorelli · 2023-02-28T10:32:20Z

I am generally +1 on this change.

Thanks!

Gonna do a @pandas-dev/pandas-core ping then to hear people's opinions

Motivation for why to do this is to further reduce merge conflicts, see #50274 for a concrete example of how it would help

Dr-Irv · 2023-02-28T13:22:30Z

Gonna do a @pandas-dev/pandas-core ping then to hear people's opinions

I find this style really un-natural from a coding standpoint. Using VSCode, imports are sometimes automatically inserted at the top. I understand the motivation. So I'm -1 on it, because it makes the imports look "noisy", but won't stand in the way.

Wondering if a solution is to resolve merge conflicts in backports by round-tripping through this idea. In other words, we keep the style as it currently is, and then when merging, convert the target to this style and the part being merged to this style, do the merge, and then convert the merged part back to the existing style. Not sure if this idea is even feasible.

MarcoGorelli · 2023-02-28T13:24:31Z

Using VSCode, imports are sometimes automatically inserted at the top.

Not sure what you mean, sorry - what's the issue with that? You'll need to run isort/pre-commit anyway

bashtage · 2023-02-28T13:39:28Z

I would also say it looks unnatural and I'm not aware of any human writing style that adopts this.

Do you have a sense as to how often this would reduce/avoid merge conflicts?

It feels like it would roughly on par, in terms of avoiding merge conflicts, as something like

from lib import (
    a,
    b,
    c,
)

which is already in use in lots of places.

MarcoGorelli · 2023-02-28T13:42:11Z

I give an example here: #50274 (comment) . I haven't counted how often this happens, but I opened the issue after having to manually fix imports in several backports when the conflict was just due to going from a single import to multiple

which is already in use in lots of places.

It's in place everywhere - the issue is in going from

from lib import a

to

from lib import (
    a,
    b,
)

MarcoGorelli · 2023-02-28T13:48:59Z

Perhaps a less unnatural-looking alternative would be to use force_grid_wrap = 1, which would force

from lib import (
    a,
)

even for single imports. I'll open a separate PR to demonstrate that

EDIT: here it is: #51693

Dr-Irv · 2023-02-28T14:44:19Z

Using VSCode, imports are sometimes automatically inserted at the top.

Not sure what you mean, sorry - what's the issue with that? You'll need to run isort/pre-commit anyway

I'm not sure of a specific example, but there are times where I'm writing code where I use something that has no import statement, and VSCode automatically creates the import statement in the right place at the top of the file. Having said that, you're correct that isort/pre-commit would fix that up automatically.

jbrockmendel · 2023-02-28T16:38:52Z

ew, gross.

jbrockmendel · 2023-02-28T16:40:10Z

The pain point for merge conflicts is in the whatsnew. id love to see a tool for that. (Also for filling in an issue reference when i dont know the number until i open a pr)

MarcoGorelli · 2023-02-28T16:47:35Z

the whatsnew conflicts are orthogonal- but I agree, it would be good to address those too

ew, gross.

😆 not sure why people don't like this style, but OK, I don't want to force it. Would you be OK with #51693 instead?

MarcoGorelli · 2023-02-28T17:40:39Z

closing in favour of #51693, which looks like it might have a non-zero chance of passing

(Also for filling in an issue reference when i dont know the number until i open a pr)

here you go https://ichard26.github.io/next-pr-number/

MarcoGorelli added the Code Style Code style, linting, code_checks label Dec 15, 2022

MarcoGorelli mentioned this pull request Dec 15, 2022

STYLE Reduce merge conflicts with force_single_line or force_grid_wrap=1 #50274

Closed

MarcoGorelli changed the title ~~STYLE Can we please use force_single_line in isort?~~ STYLE Reduce merge conflicts with force_single_line Dec 15, 2022

MarcoGorelli marked this pull request as draft December 15, 2022 16:16

MarcoGorelli changed the title ~~STYLE Reduce merge conflicts with force_single_line~~ (wait til 2.0) STYLE Reduce merge conflicts with force_single_line Dec 15, 2022

lithomas1 added this to the 2.0 milestone Dec 15, 2022

lithomas1 added the Blocker for rc Blocking issue or pull request for release candidate label Dec 15, 2022

github-actions bot added the Stale label Jan 20, 2023

MarcoGorelli force-pushed the force-single-line branch from 39ceeb7 to 3287ced Compare February 11, 2023 20:44

MarcoGorelli marked this pull request as ready for review February 11, 2023 20:45

MarcoGorelli changed the title ~~(wait til 2.0) STYLE Reduce merge conflicts with force_single_line~~ STYLE Reduce merge conflicts with force_single_line Feb 11, 2023

MarcoGorelli force-pushed the force-single-line branch from 0b629b9 to 5ce3a07 Compare February 15, 2023 10:28

MarcoGorelli added 3 commits February 15, 2023 14:09

force single line = true

0947bb5

add missing noqa:F401

2e0b2d2

from pandas import _testing as tm => import pandas._testing as tm

505820e

MarcoGorelli force-pushed the force-single-line branch from 5ce3a07 to 505820e Compare February 15, 2023 15:01

mroeschke removed Stale Blocker for rc Blocking issue or pull request for release candidate labels Feb 17, 2023

MarcoGorelli closed this Feb 28, 2023

This was referenced Mar 1, 2023

DEV: reduce merge conflicts by sorting whatsnew notes by issue number? #51712

Closed

DEV: reduce merge conflicts by sorting whatsnew notes by issue number? #51715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STYLE Reduce merge conflicts with `force_single_line` #50275

STYLE Reduce merge conflicts with `force_single_line` #50275

MarcoGorelli commented Dec 15, 2022 •

edited

Loading

lithomas1 commented Dec 15, 2022

MarcoGorelli commented Dec 15, 2022

phofl commented Dec 15, 2022

MarcoGorelli commented Dec 15, 2022

twoertwein commented Dec 19, 2022

MarcoGorelli commented Dec 19, 2022 •

edited

Loading

WillAyd commented Dec 20, 2022

github-actions bot commented Jan 20, 2023

MarcoGorelli commented Jan 20, 2023

MarcoGorelli commented Feb 15, 2023 •

edited

Loading

mroeschke commented Feb 15, 2023

simonjayhawkins commented Feb 22, 2023

MarcoGorelli commented Feb 28, 2023

Dr-Irv commented Feb 28, 2023

MarcoGorelli commented Feb 28, 2023

bashtage commented Feb 28, 2023 •

edited

Loading

MarcoGorelli commented Feb 28, 2023 •

edited

Loading

MarcoGorelli commented Feb 28, 2023 •

edited

Loading

Dr-Irv commented Feb 28, 2023

jbrockmendel commented Feb 28, 2023

jbrockmendel commented Feb 28, 2023

MarcoGorelli commented Feb 28, 2023 •

edited

Loading

MarcoGorelli commented Feb 28, 2023

STYLE Reduce merge conflicts with force_single_line #50275

STYLE Reduce merge conflicts with force_single_line #50275

Conversation

MarcoGorelli commented Dec 15, 2022 • edited Loading

lithomas1 commented Dec 15, 2022

MarcoGorelli commented Dec 15, 2022

phofl commented Dec 15, 2022

MarcoGorelli commented Dec 15, 2022

twoertwein commented Dec 19, 2022

MarcoGorelli commented Dec 19, 2022 • edited Loading

WillAyd commented Dec 20, 2022

github-actions bot commented Jan 20, 2023

MarcoGorelli commented Jan 20, 2023

MarcoGorelli commented Feb 15, 2023 • edited Loading

mroeschke commented Feb 15, 2023

simonjayhawkins commented Feb 22, 2023

MarcoGorelli commented Feb 28, 2023

Dr-Irv commented Feb 28, 2023

MarcoGorelli commented Feb 28, 2023

bashtage commented Feb 28, 2023 • edited Loading

MarcoGorelli commented Feb 28, 2023 • edited Loading

MarcoGorelli commented Feb 28, 2023 • edited Loading

Dr-Irv commented Feb 28, 2023

jbrockmendel commented Feb 28, 2023

jbrockmendel commented Feb 28, 2023

MarcoGorelli commented Feb 28, 2023 • edited Loading

MarcoGorelli commented Feb 28, 2023

STYLE Reduce merge conflicts with `force_single_line` #50275

STYLE Reduce merge conflicts with `force_single_line` #50275

MarcoGorelli commented Dec 15, 2022 •

edited

Loading

MarcoGorelli commented Dec 19, 2022 •

edited

Loading

MarcoGorelli commented Feb 15, 2023 •

edited

Loading

bashtage commented Feb 28, 2023 •

edited

Loading

MarcoGorelli commented Feb 28, 2023 •

edited

Loading

MarcoGorelli commented Feb 28, 2023 •

edited

Loading

MarcoGorelli commented Feb 28, 2023 •

edited

Loading