Added an optional signal parameters to SignalConnection.__init__

[canismarko]

Description

In the init method of typhos.core.SignalConnection class, it looks up the signal corresponding to its address in the signal_registry dictionary. This PR adds an optional signal=None argument to the SignalConnection.init() method. If signal is None (default), then the behavior is the same as before. However, if a signal is provided, it will be used for the rest of the connection.

Motivation and Context

This makes the SignalConnection class more flexible, and allows it to be used with other registries. In my case, I want to look up signals from the ophyd-registry package since the dictionary style registry used by Typhos doesn't meet all my use-cases. To do that, I will subclass the SignalConnection and SignalPlugin classes.

In its current implementation I have to vendor in the entire init method since self.signal is written half-way through and then used immediately. Vendoring in init is not desirable since future updates to this method will not be available making the code less maintainable.

With this proposed change, I can use super() instead:

class MySignalConnection(SignalConnection):
    def __init__(self, channel, address, protocol=None, parent=None, signal=None):
        # Assume *my_registry* is an instance of ``ophydregistry.Registry()``
        signal = my_registry.find(address)
        # Now call the original SignalConnection __init__ so we get all the Typhos goodness
        super().__init__(channel=channel, address=address, protocol=protocol, parent=parent, signal=signal)

How Has This Been Tested?

To ensure existing behavior is not affected, the tests in typhos/tests/plugins/test_core.py were run before and after this change, passing in both cases.

To test the new behavior, a new test was added to test_core.py, similar to the existing test_metadata() but where the signal object is not added to signal_registry and is instead passed in with the signal parameter.

Where Has This Been Documented?

The docstring for SignalConnection has been updated to describe the new parameter. It now includes:

In most cases, the default behavior is desired: to retrieve the signal from the signal_registry. However if the optional parameter signal is provided then this Signal object will be used instead.

Pre-merge checklist

• [ ] Code works interactively
• [ ] Code contains descriptive docstrings, including context and API
• [x] New/changed functions and methods are covered in the test suite where possible
• [ ] Code has been checked for threading issues (no blocking tasks in GUI thread)
• [x] Test suite passes locally
• [x] 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

[canismarko]

By the way, here's the implementation of how I'm using this without the change.

https://github.com/spc-group/haven/blob/mirrors/src/firefly/pydm_plugin.py

[tangkong]

On the surface this looks unobtrusive, preserving the original usage pattern. Thanks for adding a test as well! On the whole I'm ok with this change if it helps support your use case.

It's been a while since I've looked at Typhos but I do wonder, is the SignalConnection the only part of typhos you're using? By sidestepping typhos' signal registry, you prevent other parts of Typhos from using those signals.

[ZLLentz]

First off, thank you for opening a pull request.

I'm waffling back in forth in my head about whether an __init__ kwarg is the most appropriate place for this, or if typhos should explicitly implement an overridable get_signal method instead. As-implemented this framework is clear and useful both for your use case and for the test suite addition, but leaves behind a bit of code that is confusing on first read, since the expected target of this is a super() call in another library rather than any direct use of TyphosConnection. In contrast, a dedicated overridable get_signal or e.g. get_signal_from_registry has very clear intent and also allows you to override and re-use the rest of this without excessive vendoring.

Any thoughts about this? Are there other cases where it is useful to directly pass in signals to SignalConnection that I'm missing?

[canismarko]

is the SignalConnection the only part of typhos you're using? By sidestepping typhos' signal registry, you prevent other parts of Typhos from using those signals.

Yes, just this and the accompanying SignalPlugin. I actually rolled my own at first, then learned that y'all had already solved this problem and abandoned my buggy version for yours.

[canismarko]

Any thoughts about this? Are there other cases where it is useful to directly pass in signals to SignalConnection that I'm missing?

I had a similar thought, though I considered adding an attribute:

class SignalConnection(PyDMConnection):
    registry = signal_registry

    def __init__(self, ...):
        ...
        self.signal = self.registry[address]
        ...

then for any subclasses it's just

class MySignalConnection(SignalConnection):
    registry = my_registry

This does mean implementing __get__ on the ophydregistry.Registry class, but I'm fine with adding that (I might do it regardless).

I went with the init attribute because it seems more flexible, but I don't have a particular use case in mind. Like maybe somebody wants to create a signal and then the PyDMConnection directly? Not sure why you'd want that, though. I'm happy to adjust this to work either of the three ways:

1) signal as an init parameter 2) get_signal as an overridable method 3) registry as a class attribute

I think I like 2) best for the reasons you described.

Thoughts?

[tangkong]

I like 2, it's a good point

[ZLLentz]

I went with the init attribute because it seems more flexible, but I don't have a particular use case in mind. Like maybe somebody wants to create a signal and then the PyDMConnection directly? Not sure why you'd want that, though.

This is the only thing holding me back from insisting on 2 vs merging this as-is. It was clearly pretty useful for writing a succinct unit test, which implies that it could have further utility.

[canismarko]

I implemented a find_signal() method ("get_signal()" sounded too much like an accessor, which it isn't). I can also roll it back if we feel the flexibility of the init parameter is worth the debt of mixing concerns.

[canismarko]

Thank you both.