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

Add ability to cull terminals and track last activity #5372

Merged
merged 7 commits into from May 18, 2020

Conversation

kevin-bates
Copy link
Member

This adds functionality to track a terminal's last activity and optionally cull terminals that have been inactive for some specified duration. Similar to culling kernels, culling terminals is configured by setting the parameter --TerminalManager.cull_inactive_timeout to a value greater than 0 (default is 0 - not enabled).

The implementation introduces a TerminalManager class which is a subclass of terminado's NamedTermManager class. The handlers have been updated to work with this terminal manager.

Here are the configurable options of the TerminalManager class...

TerminalManager options
-----------------------
--TerminalManager.cull_inactive_timeout=<Int>
    Default: 0
    Timeout (in seconds) in which a terminal has been inactive and ready to be
    culled. Values of 0 or lower disable culling.
--TerminalManager.cull_interval=<Int>
    Default: 300
    The interval (in seconds) on which to check for terminals exceeding the
    inactive timeout value.

This change also updates the REST API such that both name and last_activity are returned in the JSON response. As a result, this may be helpful in addressing #1834.

I don't see any issues in Notebook's display and handling of terminals with the additional field and I'm assuming it's just ignored. However, this kind of thing may be considered an API break, although, given its only in responses, most applications can tolerate this.

Resolves: #3868

This adds functionality to track a terminal's last activity and
optionally cull terminals that have been inactive for some specified
duration.

Resolves: jupyter#3868
@kevin-bates kevin-bates mentioned this pull request Apr 15, 2020
@kevin-bates kevin-bates changed the title Add ability to cull terminals and track last activity [WIP] Add ability to cull terminals and track last activity Apr 15, 2020
@kevin-bates
Copy link
Member Author

I should also note that I didn't introduce any kind of execution_state because it seems the code is more of an echo-er and I believe you'd need to introduce a more formal protocol - like that of kernels. As a result, if a given operation were performed within a terminal session and that operation produced no output for the more than the duration of the cull_inactive_timeout, that terminal session would be culled mid-operation. This behavior is analogous to kernel culling where both cull_connected and cull_busy are True. Caveat emptor.

@kevin-bates
Copy link
Member Author

Moving to WIP to add auto-shutdown capabilities similar to kernels when the server is requested to shutdown.

@kevin-bates kevin-bates changed the title [WIP] Add ability to cull terminals and track last activity Add ability to cull terminals and track last activity Apr 15, 2020
@kevin-bates kevin-bates requested a review from takluyver May 5, 2020 15:35
@kevin-bates kevin-bates mentioned this pull request May 5, 2020
24 tasks
@blink1073 blink1073 added this to the 6.1 milestone May 15, 2020
Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

@kevin-bates, this looks great to me (I love how clean the Terminal Manager API is). Just some minor, nitpicky comments.

Are you planning to write tests that check the culling mechanism?

Currently, there are no unit tests for terminals (probably because they are often flaky), and terminals won't affect the main app, so I'm fine merging without tests. We added terminal tests in jupyter_server, so we can port this PR there and write simple tests over there. Let me know what you think.

notebook/terminal/terminalmanager.py Outdated Show resolved Hide resolved
notebook/terminal/terminalmanager.py Show resolved Hide resolved
@kevin-bates
Copy link
Member Author

Thanks for the review @Zsailer. You're right about the test (or lack of). Since there weren't any previously I didn't go down that road for this but would be willing to look into this in jupyter_server (if that's okay for the time being).

I'll address the other issues in an update.

@kevin-bates
Copy link
Member Author

@Zsailer - I've gone ahead and added some basic testing of the new terminal manager and culling. I then noticed (and should have remembered) that jupyter_server has some terminal tests already (awesome) - so those tests can be updated when this is ported to JS.

I suspect the reason there weren't tests in notebook is because this functionality is getting tested in terminado, so I think it's sufficient with just exercising the terminal manager functionality right now.

@Zsailer
Copy link
Member

Zsailer commented May 18, 2020

@kevin-bates, the tests for the TerminalManager look great! Thank you for crafting those.

(Jupyter Server needed some terminal REST API tests, since were passing some extra params through the REST API.)

@Zsailer
Copy link
Member

Zsailer commented May 18, 2020

I don't see any issues in Notebook's display and handling of terminals with the additional field and I'm assuming it's just ignored. However, this kind of thing may be considered an API break, although, given its only in responses, most applications can tolerate this.

I think a short (separate) section in the changelog mentioning that a REST API change happened should be enough to cover this "breaking" change. (Like you) I assume that most Jupyter client/frontend applications don't validate every field, so the extra fields will be ignored.

notebook/notebookapp.py Outdated Show resolved Hide resolved
@Zsailer Zsailer self-requested a review May 18, 2020 16:40
@Zsailer
Copy link
Member

Zsailer commented May 18, 2020

This looks good to me! Thanks, @kevin-bates.

@Zsailer Zsailer merged commit 3ec00ad into jupyter:master May 18, 2020
@takluyver
Copy link
Member

BTW, was it deliberate that this made terminado a hard requirement? Previously, it would catch an ImportError and the UI would show that terminals were unavailable. Now notebookapp can't be imported without terminado available. See #5462.

@kevin-bates
Copy link
Member Author

hi Thomas - it's great to hear from you.

No, it wasn't deliberate by any means - that should be evident by looking at the changeset. The act of wrapping terminado in a local manager class, coupled with providing users help options for enabling culling, inadvertently (and unknowingly) affected the runtime handling of teminado's presence. I will make this my focus today.

@Zsailer
Copy link
Member

Zsailer commented May 20, 2020

Oops, thanks, @takluyver. I didn’t know we made terminado an optional dependency either. Apologies for missing this in the review, Kevin.

@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/terminal-manager-cull-properties-not-being-applied-to-notebook-config/6222/2

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cull idle terminals
5 participants