labstreaminglayer / pylsl

Python bindings (pylsl) for liblsl
MIT License
142 stars 58 forks source link

resolve_stream should have a timeout parameter #35

Open jdevoldere opened 4 years ago

jdevoldere commented 4 years ago

As of now you can't specify a timeout which means a python program can get stuck indefinitely if no stream is found.

dmedine commented 4 years ago

Unfortunately, this is not so simple to do without breaking the consistency that exists between the APIs for various programming languages. Some languages (and operating systems) have very different mechanisms for this. One option would be to add this feature to the C++ functions that sit at the bottom of each API. Of course, this still means updating each API to reflect the changes. I would be open to adding this to the TODO list and indeed it might already be on the list of features that some developers wish to implement, but in any case it will take some time before it is ever done.

In the meantime, you can try rolling your own: https://stackoverflow.com/questions/492519/timeout-on-a-function-call

agricolab commented 4 years ago

I agree with @dmedine that it is better (i.e. likely way easier and safer) to implement this at the level of the wrapping language.

Besides, on my machine and in my experience, if no stream is present, pylsl.resolve_stream returns immediatly with an empty list, i.e []. Can you give a minimum example where it blocks if no stream is found?

jdevoldere commented 4 years ago

@dmedine this library is probably a better and easier solution for such cases: https://pypi.org/project/func-timeout/

But after looking through the source code I've realized that resolve_stream is a legacy function (despite still being used in the pylsl examples) that points towards the newer resolve_byprop function which does have a timeout parameter, but resolve_stream only allows for 3 arguments to be passed so it excludes the timeout argument.

So really the solution is to just use resolve_byprop directly.

@agricolab I think you are confusing resolve_stream with the resolve_streams function.

agricolab commented 4 years ago

The issue title says resolve_stream should have a timeout parameter, and i meant resolve_stream. But both functions return an empty list, if no streams are available. I found a working example though for your issue. You have to specify arguments, then it blocks., i.e

resolve_stream()
# returns []
resolve_stream('type','EEG')
# blocks

That is certainly unexpected behavior, and i would have expected that it returns immediately with an empty list, too.

jdevoldere commented 4 years ago

@agricolab ah it defaults to resolve_streams depending on the arguments.

def resolve_stream(*args):
    if len(args) == 0:
        return resolve_streams()
    elif type(args[0]) in [int, float]:
        return resolve_streams(args[0])
    elif type(args[0]) is str:
        if len(args) == 1:
            return resolve_bypred(args[0])
        elif type(args[1]) in [int, float]:
            return resolve_bypred(args[0], args[1])
        else:
            if len(args) == 2:
                return resolve_byprop(args[0], args[1])
            else:
                return resolve_byprop(args[0], args[1], args[2])
agricolab commented 4 years ago

Great. When resolve_stream either calls resolve_streams or resolve_byprop and both can be called in non-blocking mode, it should be trivial to add a timeout or default to non-blocking calls.

I would prefer to go non-blocking by default. But i wrote my own wrapper anyways, as i like a call by kwargs, so there's that.

def get_streaminfos_matching(**kwargs) -> List[StreamInfo]:
    """
    args
    ----
    **kwargs:
        keyword arguments to identify the desired stream

    returns
    -------
    sinfos: List[StreamInfo]
        a list of StreamInfo matching the kwargs

    Example::

        sinfos = get_streaminfos_matching(name="Liesl-Mock-EEG")
    """
    # find all available streams, check whether they are fitting with kwargs
    available_streams = pylsl.resolve_streams()
    fitting_streams = []
    if len(available_streams) == 0:
        return fitting_streams

    for st in available_streams:
        for k, v in kwargs.items():
            if eval("st." + k + "()") != v:
                break
        else:
            fitting_streams.append(st)

    return fitting_streams
agricolab commented 4 years ago

After some consideration, i think i would agree with @jdevoldere that resolve_stream behaves unexpectedly. I would suggest to reimplement it in a manner that it does not block and returns immediately with an empty list if there is no stream fitting the arguments, whether arguments were supplied or not.

One alternative is to change the signature. In that case a signature and behaviour similar to queue.get, i.e. defaulting to queue.get(block=True, timeout=None). I am worried though that this will not be threadsafe in some environments which share the underlying dll. Therefore, a non-blocking default with an example in the docstring how to use this in a thread-safe blocking manner would be my suggestion.

jdevoldere commented 4 years ago

@agricolab resolve_stream is a legacy function and is hence just in place to prevent older projects from breaking. So I think that rather than update resolve_stream we should just update the documentation (mainly the python examples).