Skip to content
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.

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

Support stdin output #233

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

davidbrochart
Copy link
Collaborator

@davidbrochart davidbrochart commented May 16, 2024

No description provided.


{
"state": Map[
"pending": bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are no non-pending inputs, they get cleared away once the result was submitted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though maybe there will be fore example if the kernel dies. Should we then clear the input box or keep it with some kind of disconnected icon?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By "pending" I meant that the input is in a "being edited" state or a "submitted" state, in which case it cannot be edited again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though maybe there will be fore example if the kernel dies. Should we then clear the input box or keep it with some kind of disconnected icon?

In this case I guess we should just remove the stdin output from the shared model, because the kernel is not actually waiting for an input anymore, right?

.. code-block:: json

{
"state": Map[
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a question we do not need to answer now: should there be an owner of the input box too? So that the input can be provided only by the user who executed the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we think inputs are collaborative, then anybody should be able to fill them, right?
Otherwise I don't think this is really compatible with CRDTs, that are essentially shared structures.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A teacher may want students to see what they are typing, but not allow students to type in into their box.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first sight it looks like the same issue as with cell code: there is no way to prevent a student from changing the code. Why would it be different with a input? If it's because it's the teacher who executed the cell, well this information is not present in the shared model, and I don't think it should be. And anyway, a student can re-execute the cell, and now should they be able to type in into the box? It doesn't really make sense.
A clearer model would be that the whole notebook is read-only for students. In this case we could ignore their Y-updates so that they can't change the shared model. This would make use of user permissions.
This is a general constraint of CRDTs, everybody sees the same data, unlike in a server/client architecture where a client makes a request and receives a dedicated reply. For instance, paginated results are possible with HTTP but not with CRDTs.

@davidbrochart
Copy link
Collaborator Author

A frontend observing cell output changes would see the addition of an output of type stdin, and consequently show an input box.
In JupyterLab, we could use a CodeMirror text editor, which would have the advantage of having the awareness for free (and show user cursors). The stdin output has an id, which we use to create a WebSocket connector to the room in the backend with this id.
Along with the text editor, there should be a "submit" button. Once submitted, the text editor is removed and the plain text shown instead.

@krassowski
Copy link
Collaborator

we could use a CodeMirror text editor

As long as we configure it to be 1-line editor this would not be a problem. input() does not accept multi-line. There is also a question of latency and performance. Though yes, it might be worth exploring.

@davidbrochart
Copy link
Collaborator Author

But input() is Python, while the kernel protocol for stdin is more general, and it can actually be an improvement to have multi-line support, what do you think?

@davidbrochart
Copy link
Collaborator Author

There is also a question of latency and performance.

Mmm but CRDTs are local-first, so there shouldn't be any latency for what the local user types, only for remote users, which mostly depends on the network.

@davidbrochart davidbrochart force-pushed the stdin branch 2 times, most recently from fc65622 to 90145e6 Compare May 30, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants