Skip to content

Add support for ANSI on Windows#31

Merged
adrianeboyd merged 15 commits into
explosion:masterfrom
njsmith:colorama-for-the-win
Dec 2, 2022
Merged

Add support for ANSI on Windows#31
adrianeboyd merged 15 commits into
explosion:masterfrom
njsmith:colorama-for-the-win

Conversation

@njsmith
Copy link
Copy Markdown
Contributor

@njsmith njsmith commented Nov 21, 2022

This is a belated replacement for the reverted #24.

Improvements this time around:

Moved the relevant code into upstream colorama, so it plays nicely with other packages using terminal features on Windows, like tqdm.

This also gives us basic ANSI support on any old Windows installs out there that still don't have the native ANSI support. I think they're all EOL already so not a big deal, but it's a free bonus, so why not.

Last time, we had to revert due to a bug in handling "fake" consoles like Jupyter notebooks. This time, we're using code that's already been out in the wild for ~1 month with no issues reported, so hopefully there won't be any need for emergency reverts.

CC @richardpaulhudson

This is a belated replacement for the reverted explosion#24.

Improvements this time around:

Moved the relevant code into upstream colorama, so it plays nicely with
other packages using terminal features on Windows, like tqdm.

This also gives us basic ANSI support on any old Windows installs out
there that still don't have the native ANSI support. I think they're all
EOL already so not a big deal, but it's a free bonus, so why not.

Last time, we had to revert due to a bug in handling "fake" consoles
like Jupyter notebooks. This time, we're using code that's already been
out in the wild for ~1 month with no issues reported, so hopefully there
won't be any need for emergency reverts.
@adrianeboyd
Copy link
Copy Markdown
Contributor

I'm not immediately sure what the best way is to do it, but it would be nice to have some tests for this including some direct notebook-related tests?

@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

Test failures are a bug in the upstream colorama type stub package; PR to fix it here: python/typeshed#9234

In the mean time I guess I'll use a # type: ignore...

@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

@adrianeboyd I added a smoke test for jupyter, making sure we don't crash when importing/using wasabi inside a notebook execution context, like happened before.

I don't know how to test it against a real Windows console in CI. But that's really colorama upstream's problem to worry about anyway, I think?

@njsmith njsmith force-pushed the colorama-for-the-win branch from 993b4c2 to 440774b Compare November 21, 2022 10:40
@njsmith njsmith marked this pull request as draft November 21, 2022 10:40
@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

Also converted to draft for the moment, because the typeshed folks were really quick and already merged the type stubs fix, plus they have automated machinery to do releases every 3 hours. So we should be able to drop the #type: ignore before merging as long as we wait a few hours.

@njsmith njsmith force-pushed the colorama-for-the-win branch from 440774b to 8d5ae10 Compare November 21, 2022 10:46
@adrianeboyd
Copy link
Copy Markdown
Contributor

Take a look at how this is handled in the thinc tests?

@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

How what's handled in the thinc tests? I just grepped them for jupyter, terminal and ansi and got zero hits :-)

@adrianeboyd
Copy link
Copy Markdown
Contributor

adrianeboyd commented Nov 21, 2022

Tests with nbconvert, but does nbconvert actually test what we want to test here?

@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

Oh I see, using nbconvert as a library instead of an executable. I saw that in the nbconvert docs but it didn't look any simpler and I already had the subprocess thing working, so I just left it. I can switch it over if you like that better though.

And yeah, I confirmed that nbconvert does use the special ipykernel.iostream.OutStream that was tripping us up (that's why the test notebook has print(sys.stdout) in it).

@adrianeboyd
Copy link
Copy Markdown
Contributor

The thinc tests don't have the python 3.6 issue, what's the difference?

@adrianeboyd
Copy link
Copy Markdown
Contributor

I'd rather do this in the test requirements (which I just found on github somewhere and haven't tested) than skip the test:

# Jupyter dependency that fails with python 3.6
pywinpty==2.0.2; python_version <= '3.6' and sys_platform == 'win32'
jupyter

But if just installing nbconvert doesn't have this problem, then maybe that would be easier?

@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

It looks like the thinc tests have a stripped down set of packages and also they're pinned to several-year-old versions:

https://github.com/explosion/thinc/blob/2310d4e1f860ba935c6f2b3bd96c0eb99ccb98ab/requirements.txt#L32-L35

Not sure which part matters, but yeah it looks like we can get the jupyter test working on 3.6. I'll fiddle around and see what works.

@njsmith njsmith marked this pull request as ready for review November 21, 2022 13:34
@njsmith
Copy link
Copy Markdown
Contributor Author

njsmith commented Nov 21, 2022

Okay, I think this is good to go?

Comment thread wasabi/tests/test_jupyter.py Outdated
Copy link
Copy Markdown
Contributor

@richardpaulhudson richardpaulhudson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I can also confirm it works on the Windows laptop to which I have access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants