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

Increase scrollback to 1000 #143

Merged
merged 3 commits into from
Feb 18, 2022
Merged

Increase scrollback to 1000 #143

merged 3 commits into from
Feb 18, 2022

Conversation

yusufbashi
Copy link
Contributor

Alleviates jupyterlab/jupyterlab#10633.
A full solution should take the users choice of scrollback history length into consideration rather than setting a constant value of "maxlen" on line 44.

Alleviates jupyterlab/jupyterlab#10633.
A full solution should take the users choice of scrollback history length into consideration rather than setting a constant value of "maxlen" on line 44.
@Wh1isper
Copy link
Contributor

As the author of #127, I had considered this when submitting this PR

  1. Do we really need to save all the output of the terminal, do people want the terminal to be like a file with all the historical information available as long as the session exists. Or do people use the terminal as a tool and don't care much about saving its output (because it can be used inside ipynb ! magic)
  2. If it is necessary to save the terminal output, what kind of approach should be taken and how much output should be saved. Because the form of buffer is based on the number of messages, the user cat a very long file, or echo a very short message actually occupy only one cache, but the network transmission is different (if too much output is saved, the next time it is opened, the user's network overhead may be very high, which will be very painful for users with little bandwidth)

Therefore, for this problem, I think you can appropriately adjust the maxlen to 200 or 500 or more, but over 10,000 may not be a good choice.

@yusufbashi
Copy link
Contributor Author

I dropped the value to 800 and it still seems to greatly alleviate the issue I linked.
800 is most likely low enough to accommodate users with low bandwidth, but if any bandwidth issues come up we should be able to decrease it further without any ill effects.

@fcollonval
Copy link

I think for those kind of need the best answer is no absolute answer - aka making it configurable.

See the best would be to add a new setting in term_settings

def __init__(self, shell_command, server_url="", term_settings={},

That can be passed to PtyWithClients constructor

def __init__(self, argv, env=[], cwd=None):

It will then be possible to add a new configurable trait in Jupyter server to tune the value (transferring it to term_settings).

https://github.com/jupyter-server/jupyter_server/blob/745f5ba3f00280c1e1900326a7e08463d48a3912/jupyter_server/terminal/terminalmanager.py#L21

@Wh1isper
Copy link
Contributor

I think for those kind of need the best answer is no absolute answer - aka making it configurable.

See the best would be to add a new setting in term_settings

def __init__(self, shell_command, server_url="", term_settings={},

That can be passed to PtyWithClients constructor

def __init__(self, argv, env=[], cwd=None):

It will then be possible to add a new configurable trait in Jupyter server to tune the value (transferring it to term_settings).

https://github.com/jupyter-server/jupyter_server/blob/745f5ba3f00280c1e1900326a7e08463d48a3912/jupyter_server/terminal/terminalmanager.py#L21

I agree. The user can decide how much content to cache, especially since Jupyterhub and other similar sites have

We can implement this in jupyter_server_terminal, configure terminado when jupyter_server_terminal is enabled

@fcollonval
Copy link

We can implement this in jupyter_server_terminal, configure terminado when jupyter_server_terminal is enabled

Thanks for pointing that project (I was not aware of it). Is the plan to make jupyter_server depends on it to avoid code duplication?

@Wh1isper
Copy link
Contributor

Wh1isper commented Feb 17, 2022

We can implement this in jupyter_server_terminal, configure terminado when jupyter_server_terminal is enabled

Thanks for pointing that project (I was not aware of it). Is the plan to make jupyter_server depends on it to avoid code duplication?

Yes, you can refer this jupyter-server/jupyter_server#651

jupyter_server_terminal is designed to replace the terminals in jupyter_server

@blink1073
Copy link
Contributor

The default in iterm2 is 1000, how about we start with that?

@blink1073 blink1073 added the bug label Feb 18, 2022
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! Windows failures are unrelated

@blink1073 blink1073 merged commit 2fe1760 into jupyter:main Feb 18, 2022
@blink1073 blink1073 changed the title Update management.py Increase scrollback to 1000 Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants