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

Jira ticket ECS-5104: Automatically close hutch-python sessions that have been idle for 48 hours or more. #383

Closed
wants to merge 4 commits into from

Conversation

janeliu-slac
Copy link
Contributor

@janeliu-slac janeliu-slac commented May 29, 2024

Summary

This PR adds a timer that automatically closes a hutch-python session that has been idle for 48 hours or more. The timer is implemented as a class called IPython Session Timer that runs in a daemon thread. Using the 'post_run_cell' hook a timestamp is created after every user interaction in the interpreter. This timestamp is compared with the maximum amount of time a session can be idle. If 48 hours have passed, the thread will close the hutch-python session.

Current Issues

I wasn't able to find a way for the ipython interpreter to exit cleanly. I was hoping to get the main thread (the ipython interpreter) to go through standard shutdown procedures and exit the program. The best solution I found is to use get_ipython().ask_exit(). Other methods I tried caused problems (more details in section below).

After get_ipython().ask_exit() runs, the interpreter will display a second prompt. When the user types something at the prompt, ipython will run the code and then exit. This behavior doesn't seem ideal. This can be tested out by running hutch-python in my rhel7 folder at /cds/home/j/janeliu/git/hutch-python.

Other methods to close ipython

I tried a couple of other methods to close the interpreter:

os.kill(os.getpid(), signal.SIGINT) - this raises a KeyboardInterrupt signal and is supposed to allow the main thread to follow standard shutdown procedures, but it generates an error message "KeyboardInterrupt escaped interact()" in hutch-python and the interpreter won't exit.

os._exit(1) - this command doesn't go through standard shutdown procedures. It does force ipython to exit, but my terminal freezes up afterwards. I think it's incorrectly terminating other processes.

Where Has This Been Documented?

This feature was discussed by maintainers of ipython, but it doesn't seem like it was ever implemented: ipython/ipython#9944

Pre-merge checklist

  • Code works interactively (a real hutch config file can be loaded)
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@janeliu-slac janeliu-slac requested review from ZLLentz and tangkong May 29, 2024 17:43
@tangkong
Copy link
Contributor

tangkong commented May 29, 2024

A couple of quick notes before diving into code (really just explaining the PR checklist):

  • We usually advocate working on a named branch on your fork, rather than the master branch. This isn't a showstopper but it'll help you keep organized
  • There are some pre-commit checks that are failing. These help keep style consistent across our repositories, as well as doing some basic static analysis. You can install these with pre-commit install inside the top-level of a repository, and they'll run before you commit anything locally. (pre-commit is installed in pcds_conda, but is easily pip-installable inside your local environment too)
  • Another little thing is pre-release notes. This helps us auto-generate detailed release notes with every tag. run ./docs/pre-release-notes.sh <issue number> <description, typically branch name> and fill out the generated file.

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Overall I think this works as intended, at least in my interactive testing 👍

Some tests would be good here, possibly monkeypatching the max_time to speed the timeout process up.


def __init__(self, ipython):
self.curr_time = 0
self.max_idle_time = 172800
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be good to expose a way for the user to set this. That'd also help us test this feature.

self._get_time_passed()

# Close the user session
print("This hutch-python session has timed out. Please start a new session.")
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to also use the logger here. Currently this doesn't show up in our log files at all, so the session will terminate and uninformed people won't know what happened.

print("This hutch-python session has timed out. Please start a new session.")

# Close this ipython session
get_ipython().ask_exit()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not sure if there's a better way. Currently if the session times out, we can actually still request a close with Ctrl+D, though it doesn't matter what we choose.
image

Copy link
Member

Choose a reason for hiding this comment

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

Taking inspiration from the linked github thread, we apparently need to follow this up with another exit call to kill the prompt that's waiting for input, otherwise as you say in the desc this waits for user input before exiting.

ip = get_ipython()
ip.ask_exit()
ip.pt_app.app.exit()


Parameters
----------
ip: ``ipython`` ``Shell``
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is copied from ipython_log.py, but the parameter name is wrong and the type hint is... odd? It should probably be the

ipython : ``IPython.terminal.interactiveshell.TerminalInteractiveShell``

from above

Copy link
Member

Choose a reason for hiding this comment

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

The weird thing about IPython is that there are dozens of shell types

self._timer(self.max_idle_time - self.idle_time)
self._get_time_passed()

# Close the user session
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's some way we can warn the user at regular intervals, e.g. "this session will time out in 2 hours if there is no further input"

@janeliu-slac janeliu-slac closed this by deleting the head repository Jun 21, 2024
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.

3 participants