Disable 80 col hard wrapping in mypy output #16488

huonw · 2022-08-12T05:57:05Z

This fixes part of #16451 by ensuring that the mypy output isn't truncated/hard-wrapped at all. In particular: the terminal width is forcibly set to be very wide ("infinite") because soft-wrapped display in a wide terminal is nicer than it being hard-wrapped/truncated to 80 cols.

This seems to only be possible via an undocumented environment variable: MYPY_FORCE_TERMINAL_WIDTH: https://github.com/python/mypy/blob/03638dd670373db0b8f00cc3bcec256d09729d06/mypy/util.py#L468

huonw · 2022-08-12T05:58:57Z

src/python/pants/backend/python/typecheck/mypy/rules.py

@@ -319,6 +321,8 @@ async def mypy_typecheck_partition(
    env = {
        "PEX_EXTRA_SYS_PATH": ":".join(all_used_source_roots),
        "MYPYPATH": ":".join(all_used_source_roots),
+        "MYPY_FORCE_COLOR": "1" if global_options.colors else "0",
+        "MYPY_FORCE_TERMINAL_WIDTH": str(shutil.get_terminal_size().columns),


I'm not clear if this is appropriate in two ways:

will this ever be running in the pantsd, where it may not have access to the real tty (and/or have an outdated understanding of the size)?

I note that there's a pants.util.docutil.terminal_width function, but that seems to be targeted at help text? Should I be using it here?

pants/src/python/pants/util/docutil.py

Lines 15 to 16 in 073062a

def terminal_width(*, fallback: int = 96, padding: int = 2) -> int:

return shutil.get_terminal_size(fallback=(fallback, 24)).columns - padding

pantsd propagates the TTY, so this should work as is.

But there is another thing to consider here, which is that including this information makes the cache key of the process dependent on your terminal width and the value of the --color flag, which isn't ideal.

For color in particular, we apparently do the same thing for pytest... which is also not great. Instead, what would likely be better to do for color in particular (for both pytest and mypy) would be to always request it in the consuming process, but then strip it (via e.g. https://stackoverflow.com/questions/14693701/how-can-i-remove-the-ansi-escape-sequences-from-a-string-in-python) from stdout/stderr afterwards before creating the CheckResult.

Terminal width is a separate issue: it cannot be adjusted post-facto. One option would be to hardcode it to e.g. 100 based on a global option (so that folks could fiddle with it in case the default wasn't good). Another option (which would be more magical but maybe totally fine) would be to round the terminal width down to the nearest 10 or 20 (or to 80 as a minimum) so that everyone with a terminal width within 10 or 20 characters of one another could still hit the cache.

Hm, that's a real bummer on terminal width...it's going to be a barrier to "desktop caching" working well, where remote cache can be shared amongst several users.

I like your suggestion for post-processing colors for both MyPy and Pytest.

Hm, that's a real bummer on terminal width...it's going to be a barrier to "desktop caching" working well, where remote cache can be shared amongst several users.

With a sufficiently granular rounding threshold (probably more like down to the nearest 40 or maybe even 80 characters), the hitrate would still be good. It's just annoyingly magical.

Still, even for local caching I'm not sure about it. If I expand my terminal because I went from 13" laptop to ultrawide monitor, I would not expect MyPy to be invalidated. I actually make that change a lot!

Perhaps we don't want to land the terminal width change yet?

Thanks for the discussion.

I'll make the change to mypy to set MYPY_FORCE_COLOR=1 always, and then call colors.strip_color (but let me know if you'd prefer pants to write its own form) or not, depending on the colour option. To keep this PR focused (since I have limited time to work on it 😄 ), I won't touch the pytest code here.

For terminal width, one option might be to hardcode to 'infinity' (9999999 or something), essentially disabling mypy's hard-wrapping entirely by default. This won't work for pytest, but should work for mypy: I'll do some experiments to confirm.

the terminal can soft-wrap diagnostic text just fine

source code typically is typically natively hard-wrapped to some manageable limit already (80, 100, 120 characters)

IIRC, this is more along the lines of what other compilers do anyway (at least 1), e.g. rustc in a 60 char terminal:

I think the infinity approach is really promising, as long as the terminal indeed will do soft wrapping. Imo, that's preferable to a global option for MyPy, which is annoying to ask the user to set and still breaks caching. Thanks!

Sounds good re: Pytest, I'll open a ticket.

I've updated this along those lines. I think the output is acceptable, even for very wide code (200+ cols) in a narrow terminal:

A few notes for this new version:

I've put the color stripping into CheckResult.from_fallible_process_result, so it can be used for other checks too. I imagine this could be used in LintResult and even TestResult but it kinda seems like there's deeper sharing between those types so I haven't tried to merge them here.

I've had to set TERM too to get colors (mypy uses curses to do it 'properly' rather than the rather more convenient approach of hardcoding the escapes ala ansicolors). From skimming https://gitlab.freedesktop.org/terminal-wg/specifications/-/issues/8 and https://gitlab.freedesktop.org/terminal-wg/specifications/-/issues/11, it seemed like TERM=xterm was a reasonable lowest common denominator for terminal emulates used in practice (especially for the simple escape sequences mypy uses https://github.com/python/mypy/blob/9a9bc3b807bb11986122dd3896e6eb3b61e2ccf8/mypy/util.py#L626-L633), but I don't have much insight here, so please let me know if there's a preferred choice.

I still haven't written any tests for the actual mypy invocations, assuming the code looks reasonable, I can add something to rules_integration_test.py

I still haven't written any tests for the actual mypy invocations, assuming the code looks reasonable, I can add something to rules_integration_test.py

Ah, looks like I have to update it anyway. It'll probably have to wait to tomorrow on my end, though.

it seemed like TERM=xterm was a reasonable lowest common denominator for terminal emulates used in practice (especially for the simple escape sequences mypy uses https://github.com/python/mypy/blob/9a9bc3b807bb11986122dd3896e6eb3b61e2ccf8/mypy/util.py#L626-L633), but I don't have much insight here, so please let me know if there's a preferred choice

Ah, unfortunately this didn't work: for xterm, terminfo (at least on my macOS system) uses a more complicated escape sequence for sgr0 (reset) \x1b(B\x1b[m, instead of just some variant of the normal \x1b[0m. colors.strip_color doesn't handle the \x1b(B escape, nor do any of the regexps in the StackOverflow discussion above.

Instead of trying to patch holes like this or pull in a library like https://github.com/selectel/pyte to do it fully properly, I've just switched to running mypy with a simpler terminal emulator specified (TERM=linux).

TERM=xterm python -c 'import curses; curses.setupterm(fd=1); print(repr(curses.tigetstr("sgr0").decode()))' # '\x1b(B\x1b[m' TERM=linux python -c 'import curses; curses.setupterm(fd=1); print(repr(curses.tigetstr("sgr0").decode()))' # '\x1b[0;10m'

I've also added a basic smoke test since this took me so long to actually get working properly 😅

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood

Thanks a lot for the contribution!

stuhood · 2022-08-15T21:17:55Z

src/python/pants/backend/python/typecheck/mypy/rules.py

@@ -319,6 +321,8 @@ async def mypy_typecheck_partition(
    env = {
        "PEX_EXTRA_SYS_PATH": ":".join(all_used_source_roots),
        "MYPYPATH": ":".join(all_used_source_roots),
+        "MYPY_FORCE_COLOR": "1" if global_options.colors else "0",
+        "MYPY_FORCE_TERMINAL_WIDTH": str(shutil.get_terminal_size().columns),


pantsd propagates the TTY, so this should work as is.

But there is another thing to consider here, which is that including this information makes the cache key of the process dependent on your terminal width and the value of the --color flag, which isn't ideal.

For color in particular, we apparently do the same thing for pytest... which is also not great. Instead, what would likely be better to do for color in particular (for both pytest and mypy) would be to always request it in the consuming process, but then strip it (via e.g. https://stackoverflow.com/questions/14693701/how-can-i-remove-the-ansi-escape-sequences-from-a-string-in-python) from stdout/stderr afterwards before creating the CheckResult.

Terminal width is a separate issue: it cannot be adjusted post-facto. One option would be to hardcode it to e.g. 100 based on a global option (so that folks could fiddle with it in case the default wasn't good). Another option (which would be more magical but maybe totally fine) would be to round the terminal width down to the nearest 10 or 20 (or to 80 as a minimum) so that everyone with a terminal width within 10 or 20 characters of one another could still hit the cache.

[ci skip-rust] [ci skip-build-wheels]

stuhood

Thanks!

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano

Excellent PR: meaningful user impact, thoughtful comments, effective testing. Well done and thank you!

benjyw · 2022-08-16T04:30:49Z

Great change, thanks for this!

stuhood · 2022-08-16T23:41:29Z

Thanks again! It looks like this needs one more test fix.

[ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood · 2022-08-18T22:21:20Z

Ok: according to debugging over in #16578, the --no-colors flag is being respected in the three failing tests, but some additional un-stripped characters are still in the output, but only on Linux.

This actually smells a little bit like an issue that I saw in Byron/prodash#9 (comment): namely that to strip some of these characters, you actually need a full terminal emulator (oy). If there are no Python packages that do that, then adding a dependency on the strip_ansi_escapes crate and exposing a Python function for it might work here?

Eric-Arellano · 2022-08-18T22:33:12Z

In the interest of landing this, we could split the PR in two also and only land the line wrapping for now?

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

huonw · 2022-08-18T23:14:44Z

Ok: according to debugging over in #16578, the --no-colors flag is being respected in the three failing tests, but some additional un-stripped characters are still in the output, but only on Linux.

Ah, bummer, thanks for attempting it. I was hoping that downgrading to TERM=linux would be enough...

If there are no Python packages that do that

There's https://github.com/selectel/pyte but I have no experience with any options here, personally. I'd appreciate guidance about your preferred path forward since I am not.

In the interest of landing this, we could split the PR in two also and only land the line wrapping for now?

I've reverted all the colour parts of this, keeping only the line wrapping. I'll open a new PR with the rest in a little while.

src/python/pants/backend/python/typecheck/mypy/rules.py

Co-authored-by: Eric Arellano <[email protected]>

huonw · 2022-08-19T04:35:12Z

I'll open a new PR with the rest in a little while.

#16586 (comment)

This fixes part of pantsbuild#16451 by ensuring that the mypy output isn't truncated/hard-wrapped at all. In particular: the terminal width is forcibly set to be very wide ("infinite") because soft-wrapped display in a wide terminal is nicer than it being hard-wrapped/truncated to 80 cols. This seems to only be possible via an undocumented environment variable: `MYPY_FORCE_TERMINAL_WIDTH`: https://github.com/python/mypy/blob/03638dd670373db0b8f00cc3bcec256d09729d06/mypy/util.py#L468 [ci skip-rust]

This fixes #16451 by continuing the color half of #16488 (terminal width). It does this by forcing mypy to always include color escapes, and then stripping them out in pants, based on the [GLOBAL].colors setting. This seems to only be possible via an undocumented environment variable: `MYPY_FORCE_COLOR`: python/mypy#7771, https://github.com/python/mypy/blob/03638dd670373db0b8f00cc3bcec256d09729d06/mypy/util.py#L543 Doing this also requires setting `TERM=linux` so that curses can find appropriate info in the terminfo database. [ci skip-rust] [ci skip-build-wheels]

…build#16586) This fixes pantsbuild#16451 by continuing the color half of pantsbuild#16488 (terminal width). It does this by forcing mypy to always include color escapes, and then stripping them out in pants, based on the [GLOBAL].colors setting. This seems to only be possible via an undocumented environment variable: `MYPY_FORCE_COLOR`: python/mypy#7771, https://github.com/python/mypy/blob/03638dd670373db0b8f00cc3bcec256d09729d06/mypy/util.py#L543 Doing this also requires setting `TERM=linux` so that curses can find appropriate info in the terminfo database. [ci skip-rust] [ci skip-build-wheels]

…y-pick of #16586) (#16808) This fixes #16451 by continuing the color half of #16488 (terminal width). It does this by forcing mypy to always include color escapes, and then stripping them out in pants, based on the [GLOBAL].colors setting. This seems to only be possible via an undocumented environment variable: `MYPY_FORCE_COLOR`: python/mypy#7771, https://github.com/python/mypy/blob/03638dd670373db0b8f00cc3bcec256d09729d06/mypy/util.py#L543 Doing this also requires setting `TERM=linux` so that curses can find appropriate info in the terminfo database. [ci skip-rust] [ci skip-build-wheels]

huonw commented Aug 12, 2022

View reviewed changes

Propagate color/terminal width to mypy

53635c6

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

huonw force-pushed the bugfix/16451-force-mypy branch from 89645e9 to 53635c6 Compare August 12, 2022 06:06

stuhood added the category:new feature label Aug 15, 2022

stuhood reviewed Aug 15, 2022

View reviewed changes

Eric-Arellano mentioned this pull request Aug 15, 2022

Fix Pytest caching being dependent on --colors option #16526

Closed

huonw added 3 commits August 16, 2022 11:59

Allow CheckResult to have formatting stripped

1f5ae08

[ci skip-rust] [ci skip-build-wheels]

Have mypy always show colors, post-process to remove if required

4139a8d

[ci skip-rust] [ci skip-build-wheels]

Set mypy's terminal width to an 'infinite' fixed value

c7a93f9

[ci skip-rust] [ci skip-build-wheels]

stuhood approved these changes Aug 16, 2022

View reviewed changes

Set TERM too, or else mypy is still non-colored

33b5a74

[ci skip-rust] [ci skip-build-wheels]

Eric-Arellano approved these changes Aug 16, 2022

View reviewed changes

huonw added 2 commits August 18, 2022 08:45

Switch to TERM=linux to avoid \033(B added to simple \033[0m 'reset'

32f394f

[ci skip-rust] [ci skip-build-wheels]

Add test for mypy's colors and disabled line wrapping

11d2bde

[ci skip-rust] [ci skip-build-wheels]

huonw changed the title ~~Propagate color/terminal width to mypy~~ Show colors and disable line wrapping in mypy output Aug 18, 2022

Eric-Arellano approved these changes Aug 18, 2022

View reviewed changes

stuhood enabled auto-merge (squash) August 18, 2022 03:25

stuhood disabled auto-merge August 18, 2022 03:25

stuhood enabled auto-merge (squash) August 18, 2022 03:25

Set --no-colors explicitly in some tests.

79948a6

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood disabled auto-merge August 18, 2022 21:48

stuhood mentioned this pull request Aug 18, 2022

Mypy color debugging #16578

Closed

huonw added 2 commits August 19, 2022 09:13

Revert color changes, keep only terminal width

9d028b3

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Reindent

0763302

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

huonw changed the title ~~Show colors and disable line wrapping in mypy output~~ Show disable line wrapping in mypy output Aug 18, 2022

huonw changed the title ~~Show disable line wrapping in mypy output~~ Disable 80 col hard wrapping in mypy output Aug 18, 2022

Eric-Arellano approved these changes Aug 18, 2022

View reviewed changes

src/python/pants/backend/python/typecheck/mypy/rules.py Outdated Show resolved Hide resolved

Update src/python/pants/backend/python/typecheck/mypy/rules.py

29a6519

Co-authored-by: Eric Arellano <[email protected]>

Eric-Arellano enabled auto-merge (squash) August 19, 2022 00:19

Eric-Arellano merged commit 7074e84 into pantsbuild:main Aug 19, 2022

huonw deleted the bugfix/16451-force-mypy branch August 19, 2022 04:20

huonw mentioned this pull request Aug 19, 2022

Force colors from mypy always, strip in pants when --no-colors #16586

Merged

stuhood mentioned this pull request Sep 9, 2022

Force colors from mypy always, strip in pants when --no-colors (Cherry-pick of #16586) #16808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable 80 col hard wrapping in mypy output #16488

Disable 80 col hard wrapping in mypy output #16488

huonw commented Aug 12, 2022 •

edited

Loading

huonw Aug 12, 2022 •

edited

Loading

stuhood Aug 15, 2022

Eric-Arellano Aug 15, 2022

stuhood Aug 15, 2022 •

edited

Loading

Eric-Arellano Aug 15, 2022 •

edited

Loading

huonw Aug 15, 2022

Eric-Arellano Aug 15, 2022

huonw Aug 16, 2022 •

edited

Loading

huonw Aug 16, 2022

huonw Aug 18, 2022 •

edited

Loading

stuhood left a comment

stuhood Aug 15, 2022

stuhood left a comment

Eric-Arellano left a comment

benjyw commented Aug 16, 2022

stuhood commented Aug 16, 2022

stuhood commented Aug 18, 2022 •

edited

Loading

Eric-Arellano commented Aug 18, 2022

huonw commented Aug 18, 2022

huonw commented Aug 19, 2022

	def terminal_width(*, fallback: int = 96, padding: int = 2) -> int:
	return shutil.get_terminal_size(fallback=(fallback, 24)).columns - padding

Disable 80 col hard wrapping in mypy output #16488

Disable 80 col hard wrapping in mypy output #16488

Conversation

huonw commented Aug 12, 2022 • edited Loading

huonw Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

stuhood Aug 15, 2022

Choose a reason for hiding this comment

Eric-Arellano Aug 15, 2022

Choose a reason for hiding this comment

stuhood Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Eric-Arellano Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

huonw Aug 15, 2022

Choose a reason for hiding this comment

Eric-Arellano Aug 15, 2022

Choose a reason for hiding this comment

huonw Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

huonw Aug 16, 2022

Choose a reason for hiding this comment

huonw Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood Aug 15, 2022

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

benjyw commented Aug 16, 2022

stuhood commented Aug 16, 2022

stuhood commented Aug 18, 2022 • edited Loading

Eric-Arellano commented Aug 18, 2022

huonw commented Aug 18, 2022

huonw commented Aug 19, 2022

huonw commented Aug 12, 2022 •

edited

Loading

huonw Aug 12, 2022 •

edited

Loading

stuhood Aug 15, 2022 •

edited

Loading

Eric-Arellano Aug 15, 2022 •

edited

Loading

huonw Aug 16, 2022 •

edited

Loading

huonw Aug 18, 2022 •

edited

Loading

stuhood commented Aug 18, 2022 •

edited

Loading