ECS-5104 Hutch-python feature: auto-close after timeout

[janeliu-slac]

Description

PR for the IPython session timer

Motivation and Context

https://jira.slac.stanford.edu/browse/ECS-5104

How Has This Been Tested?

Haven't been able to test changes to load_config.py and rix/conf.yml. How are changes to /conf.yml file tested?

Where Has This Been Documented?

Documentation is in the file comments.

Pre-merge checklist

• Code works interactively (a real hutch config file can be loaded)
• [x ] 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

[tangkong]

There are some pre-commit fixes to address here. If you install pre-commit locally you can catch these before pushing.

Some weirdness in CI here. Looks like the passing tests grab an older version of pcdscalc, which doesn't depend on xraylib?

[tangkong]

I think we also need some unit tests here. This is the kind of feature that can be super disruptive if we ever get some kind of regression

[tangkong]

I think the logic of this is looking a lot better 👍 Stray prints and debug settings aside

[janeliu-slac]

I put together some unit tests for ipython_session_timer.py. There is probably a better way to test IPythonSessionTimer._start_session() but this was the only version I was able to get working.

[tangkong]

I think we could refine the tests, but perhaps we leave that for a follow up. We've sat on this long enough. One test I'd like to add later on is one that actually tests the timer functionality. As in: we set the timeout time to ~1s, and watch for the exit signal to be sent. But that can be a later problem.

The last two tests also don't actually assert anything, the if statements always resolve to False

I think interactively this works as advertised, and the default being at 48 hours is good enough to be minimally intrusive.

[ZLLentz]

Consolidating small nitpick suggestions into one group here

[janeliu-slac]

I added some code to class FakeIPython in test_ipython_log.py to avoid error messages for self.ip.ask_exit() and self.ip.pt_app.app.exit(). Just wanted to check if this looks right for handling these two ipython functions.

class FakeIPython:
    ...

    def ask_exit():
        pass

    class pt_app:
        
        def __init__(self):
            pass

        class app:
            
            def __init__(self):
                pass

            def exit():
                pass

[tangkong]

You could probably use unittest.mock.MagicMock to individually mock the items that you care about. This object has a bunch of helpful methods for tracking calls

from types import SimpleNamespace
from unittest.mock import MagicMock

class FakePtApp:
    app = SimpleNamespace(exit=MagicMock())

class FakeIPython:
    """A fake replacement of IPython's ``TerminalInteractiveShell``."""
    user_ns: dict[str, Any]
    events: FakeIPythonEvents

    def __init__(self):
        self.user_ns = dict(In=[""])
        self.events = FakeIPythonEvents()
        self.pt_app = FakePtApp()
        self.ask_exit = MagicMock()

later in the tests:

@unittest.mock.patch('time.sleep', lambda seconds: None)
def test_start_session_case1(session_timer, fake_ipython, capsys):
    session_timer.user_active = False

    session_timer._start_session()

    captured = capsys.readouterr()
    assert "timed out" in captured.out
    assert session_timer.ip.ask_exit.called
    assert session_timer.ip.pt_app.app.exit.called

Aside from that, the code snippet you shared doesn't look entirely right / complete. You're creating the pt_app class but never instantiating it. I think it technically works but it looks weird to me. You should be able to run the test suite locally to verify things work.

[janeliu-slac]

Thanks for the MagicMock suggestion. Going to read up on it and learn what it's doing. I've added this and the unit tests are passing. Does the hutch-python session timer feature look ready to merge?

[tangkong]

I think this looks good to me. Some hard fought lines of code but I like where it ended up! 🎉

[ZLLentz]

Three questions/thoughts about the unit tests
Overall this is great and we can probably merge soon

[ZLLentz]

fwiw I think we could definitely merge as-is, it does the thing correctly and is well-documented. I'm splitting hairs on the unit tests at this point.

[janeliu-slac]

cool, it's done! I'll merge tomorrow morning.