Interactive console in pyodide-py #1125

casatir · 2021-01-11T23:00:59Z

This is the 1/3 part of the #875 splitting. It addresses:

sync behavior
full stdout/stderr support
clean namespace
tests
bigger font (in console.html)

In a separate PR, I will address:

correct result representation
more tests

And in a last PR, I will propose a completion system.

hoodmane · 2021-01-11T23:24:01Z

Can you temporarily make this into a console2.html and leave the original console.html as is so I can easily compare the two side by side?

Stream redirection should be handled with a context manager e.g.

from contextlib import redirect_stdout, redirect_stderr
import pyodide
def run_code(code):
   with redirect_stdout(mystdoutstream), redirect_stderr(mystderrstream):
      pyodide.eval_code(code)

Exactly what you don't want is for the existence of a terminal object somewhere to funnel all writes to stdout into your terminal.

casatir · 2021-01-11T23:52:30Z

Thanks @hoodmane for looking at this.

Can you temporarily make this into a console2.html and leave the original console.html as is so I can easily compare the two side by side?

Can't understand why you need me to do this.

Stream redirection should be handled with a context manager e.g.

At first, I planed to do it like this. But ...

Exactly what you don't want is for the existence of a terminal object somewhere to funnel all writes to stdout into your terminal.

I think that it could be exactly what you want! You can stop the redirection with restore_stdstreams if you need. There can be async stuff that continue to print to stdout that you precisely want to catch.

hoodmane · 2021-01-11T23:59:19Z

I am constantly opening up the browser console and typing pyodide.runPython(stuff) into the browser console. I don't like it when messages caused by this get vacuumed up by the terminal. I think commands invoked through the terminal should log into the terminal, commands invoked elsewhere should be logged into the browser console.

Effectively there are two separate use cases: normal people and developers. I am okay if you make this stream change to console.html as long as I can have a devconsole.html that doesn't do it. Which gets back to my request to keep the current console.html around in some form.

There can be async stuff that continue to print to stdout that you precisely want to catch.

I would try to solve this in another way before going to the "alter global state" approach which I think is a last resort. Also, if the async stuff is happening on the javascript side, this won't catch it anyways.

hoodmane · 2021-01-12T00:02:24Z

One other noteworthy thing: normally if you write to stderr, when the message is printed in the browser console it gets a javascript stack trace attached. If you redirect that to the terminal and swallow the browser console message, the javascript stack trace is lost.

casatir · 2021-01-12T00:37:40Z

Thanks @hoodmane for looking at this.

I am constantly opening up the browser console and typing pyodide.runPython(stuff) into the browser console. I don't like it when messages caused by this get vacuumed up by the terminal. I think commands invoked through the terminal should log into the terminal, commands invoked elsewhere should be logged into the browser console.

Can't get your point: (after the first command is run,) stdout were already constantly redirected to terminal. I am not changing the behavior here. If you want an html page that loads Pyodide so that you can do some pyodide.runPython(stuff) without interference then console.html may not be the right tool.

There can be async stuff that continue to print to stdout that you precisely want to catch.

I would try to solve this in another way before going to the "alter global state" approach which I think is a last resort. Also, if the async stuff is happening on the javascript side, this won't catch it anyways.

It could. When implementing p5.js support in Basthon, I can have a draw function, written in Python and called from P5 that can do some async print. This is the JS side that calls some Python.

One other noteworthy thing: normally if you write to stderr, when the message is printed in the browser console it gets a javascript stack trace attached. If you redirect that to the terminal and swallow the browser console message, the javascript stack trace is lost.

Can't get any trace from pyodide.runPython("print('foo', file=sys.stderr)")

hoodmane · 2021-01-12T00:58:13Z

Can't get any trace from pyodide.runPython("print('foo', file=sys.stderr)")

Sure can:

hoodmane · 2021-01-12T01:29:39Z

If you want an html page that loads Pyodide so that you can do some pyodide.runPython(stuff) without interference then console.html may not be the right tool.

The current console.html is far from perfect but it gets the job done. I would be fine with copying it to devconsole.html and applying this stdout vacuuming semantics in console.html.

hoodmane · 2021-01-12T04:08:33Z

clean namespace

What do you mean by this? I am having trouble finding where in this code this is taken care of.

hoodmane · 2021-01-12T05:01:08Z

Can't get any trace from pyodide.runPython("print('foo', file=sys.stderr)")

Actually now that I'm looking into it, the behavior is kind of funny: printing to stderr works correctly until you execute the first command in the repl. After the first command in the repl is executed, all writes to stdout and stderr are swallowed. I can fix it to use context manager in devconsole.html so that it will work a bit more consistently.

casatir · 2021-01-12T06:28:00Z

What do you mean by this? I am having trouble finding where in this code this is taken care of.

Just do something like Console() instead of Console(locals=globals()) to let code.InteractiveConsole manage it's own namespace.

Actually now that I'm looking into it, the behavior is kind of funny: printing to stderr works correctly until you execute the first command in the repl. After the first command in the repl is executed, all writes to stdout and stderr are swallowed.

This is what I was saying by:

(after the first command is run,) stdout were already constantly redirected to terminal. I am not changing the behavior here.

This is because sys.stdout/stderr are set at runcode.

casatir · 2021-01-12T06:44:36Z

The point is that I want console.html to be a showcase and you want it to be a dev tool...

hoodmane · 2021-01-12T07:07:49Z

The point is that I want console.html to be a showcase and you want it to be a dev tool...

@casatir The point is that you don't have great reading comprehension. I said three times I wanted a tool called devconsole.html which is similar to console.html but designed to be a devtool:

I am okay if you make this stream change to console.html as long as I can have a devconsole.html that doesn't do it.

I would be fine with copying it to devconsole.html and applying this stdout vacuuming semantics in console.html.

I can fix it to use context manager in devconsole.html so that it will work a bit more consistently.

casatir · 2021-01-12T08:10:54Z

@casatir The point is that you don't have great reading comprehension. I said three times I wanted a tool called devconsole.html which is similar to console.html but designed to be a devtool:

Ouch! I was just saying that to go your way and support the fact that we need both. You start the review with a dev tool in mind but I was advertising for a showcase in #875:

It's a showcase for Pyodide

This was just to make it clear that, at first, we have different purpose in mind.

Moreover, please be kind. With this kind of answer, you will ruin the motivation of volunteers that takes on their (work and family) time for working on Pyodide and that are not fluent in english.

rth · 2021-01-12T09:03:57Z

Thanks for this improvement @casatir and for the review @hoodmane !

Moreover, please be kind.

Yes, absolutely. It's OK to have a technical disagreement, and a PR author can refuse changes suggested in a PR, while still having a pleasant conversation. As far as I understand the implied meaning was "I won't change it, unless you have better arguments" rather than literately "I don't understand". I know it's easy to misunderstand each other.

In this case, I would also prefer to keep a single console.html and address @hoodmane 's concern in this console,

Stdout were already constantly redirected to terminal. I am not changing the behavior here.

likely in a different PR, unless the changes in this PR prevent that?

hoodmane · 2021-01-12T12:01:16Z

After my repeated attempts at compromise, @casatir's tone comes across as accusatory and lacking of acknowledgement of the compromises.

This was just to make it clear that, at first, we have different purpose in mind.

"at first" is a qualification that you didn't give in your original message. It also makes your comment rather stale information, since I suggested other positions based on the stuff you've already said. It is of course true that the user I am thinking of first is myself, but I am looking for some option that can make us both happy (of course making us both happy is different than doing the best thing or even from doing a reasonable thing, but hopefully it is at least vaguely related).

The point is that I want console.html to be a showcase and you want it to be a dev tool...

This is an accusation that I don't care about end users? I'm thinking: He isn't happy with the compromise options I offered? He just doesn't want my feedback? Or did he just not read my posts very carefully and didn't notice that I changed my position at all? It sounds like a hard bargain to me.

hoodmane · 2021-01-12T12:04:09Z

in a different PR

Yes, I am now convinced by that. The current behavior is weird, this preserves the weird current behavior while making unrelated improvements.

hoodmane · 2021-01-12T14:17:23Z

The main source of my confusion in this discussion is that I've been taking the ground truth state of the system to be what happens in a fresh copy of console.html. Because the fresh copy of console.html has different behaviors from a console.html that has been used once, I had incorrect beliefs about how console.html normally works.

casatir · 2021-01-12T14:41:44Z

Thanks @rth.

It's OK to have a technical disagreement

Totally agree but it's not OK to have personal attack like this:

you don't have great reading comprehension

After my repeated attempts at compromise, @casatir's tone comes across as accusatory and lacking of acknowledgement of the compromises.

Wait wait wait.

You never suggest implementing this compromise in this PR.
My PR doesn't affect the current behavior (at least after the first command is run) on the point we are interested in.
Can't find where my tone comes across as accusatory but if I hurt you I apology. No acknowledgment from you for this work neither.
Please, check your tone.

I am looking for some option that can make us both happy (of course making us both happy is different than doing the best thing or even from doing a reasonable thing, but hopefully it is at least vaguely related).

Does this suggest I want to do wrong or unreasonable things?

This is an accusation that I don't care about end users? I'm thinking: He isn't happy with the compromise options I offered? He just doesn't want my feedback? Or did he just not read my posts very carefully and didn't notice that I changed my position at all? It sounds like a hard bargain to me.

There is no accusation here, I promise. I have no doubt you are really concern by end users. I had just not understand you proposes this compromise for this PR.

The main source of my confusion in this discussion is that I've been taking the ground truth state of the system to be what happens in a fresh copy of console.html. Because the fresh copy of console.html has different behaviors from a console.html that has been used once, I had incorrect beliefs about how console.html normally works.

Totally agree that this stupid bug makes a lot of confusion in our discussion.

Let's forget all this. I have a proposition that will certainly makes all of us happy. I am not really interested by the console.html behavior but more on the underlying pyodide.console.InteractiveConsole one. What I propose is something like that

class InteractiveConsole(code.InteractiveConsole):
    def __init__(
        self,
        locals: Optional[dict] = None,
        stdout_callback: Optional[Callable[[str], None]] = None,
        stderr_callback: Optional[Callable[[str], None]] = None,
        persistent_stream_redirection: bool = False,
    ):

The persistent_stream_redirection (maybe there is better name for it) when set to True will act as it is in this PR while it will act as you (and @rth) want when set to False. Default value will be False and it will be set to False in console.html.

WDYT?

If you are OK, I'll add this to this PR.

hoodmane · 2021-01-12T14:47:16Z

I have a proposition that will certainly makes all of us happy.
...

Sounds perfect!

Default value will be False and it will be set to False in console.html.

I don't have a super strong opinion about what the default value should be, I still think it could be a good idea to have a devconsole.html with slightly different config than console.html. But if we can only have one official entrypoint for both end users and devs, then it would be nice to have one with this set to False.

hoodmane · 2021-01-12T15:04:30Z

@casatir Sorry for the misunderstanding, I really appreciate your work! I will try not to read your tone as so confrontational in the future. This has been a really rough year and I am not exactly a pillar of emotional stability.

You never suggest implementing this compromise in this PR.

Right, I meant to imply that you could do nothing in this PR and then I would do something to deal with it in a future PR. I guess I never explicitly said that, but I was thinking I would just copy one of the old console.html's in a separate PR and make a separate devconsole.html and that would avoid you needing to support a console that is simultaneously useful as a devtool and as a showcase.

No acknowledgment from you for this work neither.

That's definitely my bad!

Please, check your tone.

I will try.

Does this suggest I want to do wrong or unreasonable things?

No sorry don't mean to suggest that. I mean that theoretically "contributors are happy" is independent from "a good decision was made", you can have either one without the other, both, or neither.

casatir · 2021-01-12T15:06:19Z

Sounds perfect!

Great!

I don't have a super strong opinion about what the default value should be,

Me neither.

I still think it could be a good idea to have a devconsole.html with slightly different config than console.html. But if we can only have one official entrypoint for both end users and devs, then it would be nice to have one with this set to False.

Could be nice to have @rth though on this.

And what about an URL parameter like ?dev=true/false or something like that. It's easy to parse with the JS URL parser in console.html and it can be hidden to standard users while advertised to devs. Then we can manage the way the console is created.

hoodmane · 2021-01-12T15:08:40Z

And what about an URL parameter like ?dev=true/false or something like that. It's easy to parse with the JS URL parser in console.html and it can be hidden to standard users while advertised to devs. Then we can manage the way the console is created.

~~I think this would be a good way to go as long as the config options are simple.~~ Actually, yes, this sounds perfect to me.

rth · 2021-01-12T22:23:31Z

src/pyodide-py/pyodide/console.py

+        """ A banner similar to the one printed by the real Python interpreter. """
+        # copyied from https://github.com/python/cpython/blob/799f8489d418b7f9207d333eac38214931bd7dcc/Lib/code.py#L214
+        cprt = 'Type "help", "copyright", "credits" or "license" for more information.'
+        return f"Python {sys.version} on {sys.platform}\n{cprt}"


This would show "Python 3.8 on emscripten". Technically it's true but it might be confusing to users. I'm not sure what else we could put there, WebAssembly VM, pyodide, browser? Also keeping the snake emoji we had before would be a nice touch.

It shows

Python 3.8.2 (default, Jan 4 2021, 12:17:49) [Clang 6.0.1 (/b/s/w/ir/cache/git/chromium.googlesource.com-external-github.com on emscripten Type "help", "copyright", "credits" or "license" for more information. >>>

which is not nice, you are right...

I see something like

Python 3.8.2 (default, Jan 4 2021, 12:17:49) on WebAssembly VM Type "help", "copyright", "credits" or "license" for more information.

for the default banner and

Python 3.8.2 (default, Jan 4 2021, 12:17:49) on WebAssembly VM Welcome to the Pyodide terminal emulator 🐍 Type "help", "copyright", "credits" or "license" for more information.

for the one customized in console.html. WDYT?

Or maybe:

Welcome to the Pyodide terminal emulator 🐍 Python 3.8.2 (default, Jan 4 2021, 12:17:49) on WebAssembly VM Type "help", "copyright", "credits" or "license" for more information.

I personally prefer the last version (with the welcome message at the beginning)

Me too, this is the one I commited.

rth · 2021-01-12T22:31:19Z

src/pyodide-py/pyodide/console.py

+
+    Parameters
+    ----------
+    stdout_callback


locals also need a docstring entry

Right. Corrected.

rth · 2021-01-12T22:35:52Z

src/tests/test_console.py

+    assert my_stderr == "bar\n"
+
+    # redirections disabled at destruction
+    shell.restore_stdstreams()


If this test fails before this line does it mean that we get monkeypatched stdout/stderr globally?

Maybe it would be good add a pytest fixure that stores and restores the stdout just in case. Something like,

@pytest.fixture def restore_stdstreams(): # save streams to local variables yield # restore streams after the test function exits def test_interactive_console_streams(restore_stdstreams): ...

BTW, it's interesting that pytest also captures stderr/stdout but does it at the level of file system descriptors https://docs.pytest.org/en/stable/capture.html#setting-capturing-methods-or-disabling-capturing

Right, it could be a bad issue!

rth · 2021-01-12T22:37:25Z

Minor comments above but otherwise LGTM. Thanks!

And what about an URL parameter like ?dev=true/false or something like that.

Also +1 for it (not in this PR).

rth · 2021-01-13T12:40:52Z

Also please add a changelog entry.

casatir · 2021-01-14T00:12:07Z

Sorry for the force push but there were conflicts with #979 so I rebased on master.

casatir · 2021-01-14T00:15:48Z

src/templates/console.html

        term.handlePythonResult = function(result) {
+          c.restore_stdstreams();
+          term.resume();


console.html is not really clean with this restore_stdstreams() in the JS code but they will disappear in the part 2/3 of the console improvement, I promise.

rth

Thanks! Please merge upstream master into this PR to resolve new conflicts.

rth · 2021-01-14T08:00:12Z

Actually I fixed conflicts via UI. Will merge on green CI.

rth · 2021-01-14T08:02:03Z

src/templates/console.html

        }

-        let term = $('body').terminal(
+        var term = $('body').terminal(


Suggested change

var term = $('body').terminal(

let term = $('body').terminal(

unless it was an intentional change?

No, probably forgotten when merging with #979.

casatir · 2021-01-14T09:05:52Z

Conflicts should be fixed. var/let too. Is there a way to rebase on master without destroying the review history?

dalcde · 2021-01-14T09:07:27Z

Merging master should be fine

casatir · 2021-01-14T09:17:24Z

Thanks @dalcde. You mean git merge upstream/master? Right, i will do this. I am a fond of clean linear history so I rebase everywhere. But merging is fine since commits will be squashed at PR merge.

rth · 2021-01-14T17:17:29Z

It's working very nicely. But now we definitely need to fix #877

rth · 2021-01-15T18:30:06Z

src/pyodide-py/pyodide/console.py

+        """ A banner similar to the one printed by the real Python interpreter. """
+        # copyied from https://github.com/python/cpython/blob/799f8489d418b7f9207d333eac38214931bd7dcc/Lib/code.py#L214
+        cprt = 'Type "help", "copyright", "credits" or "license" for more information.'
+        return f"Python {platform.python_version()} {platform.python_build()} on WebAssembly VM\n{cprt}"


This currently prints,

Python 3.8.2 ('default', 'Jan 15 2021 09:54:23') on WebAssembly VM

while the classical Python REPL displays,

Python 3.8.6 (default, Sep 25 2020, 09:36:53)

It would be nice to remove the quotes.

Arf, sorry for this. Good catch! It will be corrected in #1141. Is that OK?

Yes, sure, thanks!

rth reviewed Jan 12, 2021

View reviewed changes

casatir force-pushed the improving-console-part-1 branch from 5c4939e to a68c4cb Compare January 14, 2021 00:10

casatir commented Jan 14, 2021

View reviewed changes

casatir mentioned this pull request Jan 14, 2021

console.html should limit size of output #1135

Closed

rth approved these changes Jan 14, 2021

View reviewed changes

rth reviewed Jan 14, 2021

View reviewed changes

casatir added 13 commits January 14, 2021 10:02

Preparing console.py to adresse pyodide#875.

4fee515

Fxing console.html async behavior.

531aec7

True std streams redirection in InteractiveConsole.

8229e4c

Named stdout/stderr.

f9b187c

Mimic the CPython's banner.

dbea1c4

Bigger font in console.html.

61affb0

Testing console redirections.

9ea970f

Persistent stream redirections.

0652441

Cleanner banner.

e4ef302

Safe std stream redirection in tests.

29e6919

Add changelog entries.

4cd0344

Testing non persistent redirection.

112db8f

var => let.

808888c

casatir force-pushed the improving-console-part-1 branch from ca08d2e to 808888c Compare January 14, 2021 09:03

rth merged commit c01ce74 into pyodide:master Jan 14, 2021

casatir deleted the improving-console-part-1 branch January 14, 2021 10:35

This was referenced Jan 14, 2021

Indicate when a package is loading in CLI #695

Closed

Improving console.html #875

Closed

casatir mentioned this pull request Jan 15, 2021

Correct repr in InteractiveConsole #1141

Merged

rth reviewed Jan 15, 2021

View reviewed changes

casatir mentioned this pull request Feb 19, 2021

Use globals() for console.html #1266

Merged

hoodmane mentioned this pull request Feb 25, 2021

Bug: Syntax error handling in console.html #1286

Closed

	var term = $('body').terminal(
	let term = $('body').terminal(

Interactive console in pyodide-py #1125

Interactive console in pyodide-py #1125

Conversation

casatir commented Jan 11, 2021

hoodmane commented Jan 11, 2021 • edited

casatir commented Jan 11, 2021

hoodmane commented Jan 11, 2021 • edited

hoodmane commented Jan 12, 2021 • edited

casatir commented Jan 12, 2021

hoodmane commented Jan 12, 2021

hoodmane commented Jan 12, 2021

hoodmane commented Jan 12, 2021

hoodmane commented Jan 12, 2021

casatir commented Jan 12, 2021

casatir commented Jan 12, 2021

hoodmane commented Jan 12, 2021 • edited

casatir commented Jan 12, 2021

rth commented Jan 12, 2021 • edited

hoodmane commented Jan 12, 2021

hoodmane commented Jan 12, 2021

hoodmane commented Jan 12, 2021

casatir commented Jan 12, 2021

hoodmane commented Jan 12, 2021 • edited

hoodmane commented Jan 12, 2021 • edited

casatir commented Jan 12, 2021

hoodmane commented Jan 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth commented Jan 12, 2021 • edited

rth commented Jan 13, 2021

casatir commented Jan 14, 2021

Choose a reason for hiding this comment

rth left a comment • edited

Choose a reason for hiding this comment

rth commented Jan 14, 2021

rth Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casatir commented Jan 14, 2021

dalcde commented Jan 14, 2021 via email

casatir commented Jan 14, 2021

rth commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane commented Jan 11, 2021 •

edited

hoodmane commented Jan 11, 2021 •

edited

hoodmane commented Jan 12, 2021 •

edited

hoodmane commented Jan 12, 2021 •

edited

rth commented Jan 12, 2021 •

edited

hoodmane commented Jan 12, 2021 •

edited

hoodmane commented Jan 12, 2021 •

edited

hoodmane commented Jan 12, 2021 •

edited

rth Jan 14, 2021 •

edited

rth commented Jan 12, 2021 •

edited

rth left a comment •

edited

rth Jan 14, 2021 •

edited