search

aws-samples / sagemaker-studio-auto-shutdown-extension

Apache License 2.0
144 stars 38 forks source link

Image Terminals are shut down even though "keep_terminals"-flag is set to True #26

Closed belleny90 closed 2 years ago

belleny90 commented 2 years ago

The problem is located in idle_checker.py: https://github.com/aws-samples/sagemaker-studio-auto-shutdown-extension/blob/main/sagemaker_studio_autoshutdown/idle_checker.py

When the IdleChecker is shutting down any notebooks in its method "is_idle" because num_sessions > 0, it does not check if any terminals are open and if the "keep_terminals"-flag is set to true. As a result the whole application including terminals is shut down even though image terminals should have been kept alive.

durgasury commented 2 years ago

Hi @belleny90, can you confirm you're using the latest version of the extension? We've tested this scenario and it works as expected. In idle_checker, line, we check for the flag. If this flag is not set, it goes to the other elif statements, including the one you mentioned. Let us know if you're still facing this issue.

belleny90 commented 2 years ago

Hi @durgasury, yes I am using the latest version. I've seen that the flag is taken care of in the line you mentioned. The problem is, that the if-statement in this line also assumes num_sessions < 1. Hence, if notebooks are running we have num_sessions >= 1 and it goes to the last elif-block - no matter of num_terminals and the flag "keep-terminals". The last block does not contain the check for the flag or num_terminals and shuts down the application including image terminals after closing all notebooks.

durgasury commented 2 years ago

Gotcha! When a session AND a terminal is open, seems to kill both. If only the terminal is open, it is retained. I'll check what the design is and update the extension accordingly. Thanks for clarifying.

belleny90 commented 2 years ago

Exactly, thank you! I also opened a pull request today, which simply adds needed checks to solve this issue. However it could also be useful to reorganize the whole if/elif-logic.

durgasury commented 2 years ago

Thanks @belleny90 , I've tested the code and merged it in the PR mentioned above. Closing this issue.