Dataframe subset

[zakv]

This PR implements some of the features requested in #3 by adding optional n_sequences and filter_kwargs arguments to lyse.data().

Changes:

• Add an optional n_sequences argument to lyse.data() which allows the user to request only a subset of the lyse dataframe, which can speed execution.
  • If there are fewer than n_sequences sequences available, then the entire lyse dataframe is returned.
• Add an optional filter_kwargs argument to lyse.data()
  • This is a dictionary of keyword arguments passed to Dataframe.filter() before the dataframe is transmitted.
• Improve thread safety of Webserver.handler()
• Add a docstring for lyse.data()

[zakv]

I actually have a few questions still:

1. Previously WebServer.handler() didn't have anything special in it to make it thread safe. Is it necessary to add a lock or an inmain_decorator or something now that there is some processing of the dataframe's info in WebServer.handler()?
2. I'd be happy to implement the other feature requested in Multi-shot routines should be able to ask for a sub-set of the dataframe #3 as well, namely an optional date argument. I just have two questions related to that:
   • What form should the argument take? A string with some particular formatting? An instance of some date/time class?
   • Should this be AND'ed with n_sequences? In other words if n_sequences = 100 and date='today' then only return sequences from today, up to 100 of them.

[philipstarkey]

Yeah, I believe I've raised the thread safety issue somewhere before (doesn't seem to have its own issue though...). I'm pretty sure the existing implementation is thread unsafe because the data frame is sent to internal zprocess methods for serialisation (I think using pickle?), and it's passed by reference. So even a lock or in_main call won't help with that because the lock would always be released before zprocess serialised it.

The only way to safe with the current architecture is to do a deepcopy of the data frame and return that (assuming it's even possible to deepcopy a data frame). Performance would probably be terrible for large data frames? Maybe it's worth trying though? I also have no idea if subsets return deep copies or only shallow copies of data.

Regarding 2, what about having n_sequences and then a set of arguments to pass directly to the filter method of the data frame? Then we don't have to worry about what sort of filtering we're supporting. Maybe have a single argument for data() called filter which the user would specify as a dictionary where the key/values match the pandas method and then the lyse code can just **filter_kwargs inside the call to filter if that makes sense?

[chrisjbillington]

zprocess doesn't defer serialisation such that it might occur in a different thread - serialisation occurs in the calling thread. (zmq actually sending the data is deferred, but not serialisation via pickling). So a lock or inmain() should be sufficient I should think.

pickling holds the GIL, so the dataframe can't be modified mid-pickle. However, the dataframe that gets sent may be in the middle of being updated by update_row(), so it may have some analysis results updated and some not yet.

I think inmain_decotrator on the handler would remove that possibility, since update_row() is called in the main thread.

[philipstarkey]

Adding inmain_decorator on the handler only prevents updates to the dataframe while that method runs though. All of the machinery in ZMQServer.mainloop and ZMQ itself (where the pickling occurs right?) runs after ZMQServer.handler completes, and so the main thread can continue and may (partially) modify the dataframe before it is sent down the socket (unless we make a copy). So I don't think that solves the thread safety problem.

Also, just because pickling holds the GIL, doesn't Python still context switch between threads every 15ms anyway (previously was 1000 lines of bytecode in Python 2)? I don't think relying on the GIL is sufficient.

Both issues can be solved with a dedicated DataFrame lock, but zprocess would need to be updated so it can also be passed the lock somehow (to be held while internal zprocess/zmq code interacts with the dataframe). To be honest, I'm surprised there isn't a PEP for automatically using an RLock before accessing an object if one exists in object.__lock__. That way code like zprocess wouldn't need to grow additional kwargs to pass around lock objects...

EDIT: alternatively, if handler returns a copy of the dataframe, made while run in the main thread, then that would be safe too (since the only reference to the copy held after handler runs would be in the internal zprocess code). I was hoping to avoid data duplication though.

[chrisjbillington]

Ah I'm sorry, you're right. Would have to have the zmqserver's mainloop hold a lock somehow, not currently supported or easy to do by subclassing (but see below for an alternate method).

No, no other Python code can run whilst pickling, since pickling is a single python bytecode instruction that calls into C, and doesn't release the GIL within the C code. The whole interpreter stops whilst pickling, it's not possible for an object to be modified mid-pickle. I think this is by design and not just an implementation detail, though I'm not 100% sure.

But that's not super relevant - pickling itself can't fail, but it can begin at any arbitrary moment whilst the dataframe is being modified, so you can still get a half-modified dataframe.

EDIT: alternatively, if handler returns a copy of the dataframe, made while run in the main thread, then that would be safe too (since the only reference to the copy held after handler runs would be in the internal zprocess code). I was hoping to avoid data duplication though.

To avoid data duplication, we could add inmain_decorator to the handler, and have it call the ZMQServer's send method directly to send the response then and there instead of returning the object to be sent. Then we instead return ZMQServer.NO_RESPONSE:

@inmain_decorator(wait_for_return=True)
def handler(self, data):
    ....
    self.send(dataframe)
    return self.NO_RESPONSE
    ...

That way the pickling happens in the send() method whilst the lock is still held (edit: sorry, no lock is held per se,rather we're relying on the dataframe only being modified in the main thread. If that's not a safe assumption, we either need to make it true, or add a new lock around modifying the dataframe).

I use this same mechanism in the IMQDdxCamera BLACS tab, there it was because the handler does some time-consuming work updating the displayed image, but I didn't want that to delay the response. So it calls send() to send a response immediately before displaying the new image.

[zakv]

I've implemented Chris's suggestion with @inmain_decorator and self.send().

I also implemented Phil's filter suggestion, though I called it filter_kwargs to avoid using the name of a builtin. Sending a dictionary between the processes was a little tricky so I had to bring in a few imports: argparse, shlex, and ast. They're all from the standard library though.

I noticed that the other classes were using ParentClassName.__init__() rather than super().__init__() so I did the same with WebServer.

There is one issue that I haven't figured out yet though. For some reason the dataframe's index is replaced with a RangeIndex if filter_kwargs is used:

In []: filter_kwargs={'like':'run'}

In []: lyse.data(filter_kwargs=filter_kwargs).index
Out[]: RangeIndex(start=0, stop=3, step=1)

In []: lyse.data().filter(**filter_kwargs).index
Out[]: 
MultiIndex([('2020-10-29 19:07:50-04:00', '2020-10-29 19:07:51-04:00'),
            ('2020-10-29 19:07:50-04:00', '2020-10-29 19:07:53-04:00'),
            ('2020-10-29 19:07:51-04:00', '2020-10-29 19:07:54-04:00')],
           names=['sequence', 'run time'])

At first I assumed it had to do with the way that filter_kwargs is sent over ZeroMQ to the WebServer, but I saw the same behavior even when I temporarily hardcoded a value for filter_kwargs in WebServer.handler(). I'll keep digging but any suggestions would be appreciated.

[zakv]

Ok I think I see the issue actually. The app.filebox.shots_model.dataframe uses a RangeIndex, then when the dataframe reaches lyse.data() its changed to a multiindex. Running filter() in WebServer.handler runs it on app.filebox.shots_model.dataframe and can end up filtering out the columns that would later be used to generate the multiindex. Then lyse.data() swallows the error generated when trying to make the multiindex and just returns the dataframe with the rangeindex.

I guess the solution is to move the multiindex-generating code from lyse.data() to WebServer.handler()? That way it could be run before filter().

[philipstarkey]

Was the use of shlex etc. to maintain backwards/forwards compatibility when accessing the dataframe from a standalone script? (For example lyse v3.0 server talking to lyse v3.1 client on another PC?).

Normally I would have probably just sent something like ['get_dataframe', n_sequences, filter_kwargs] as the command passed to zmq_get (zmq_get can send python objects), but I guess that would mean new lyse clients would raise an error when talking to old lyse servers. Oh...if n_sequences is None and filter_kwargs is None, the new client could fall back to sending just 'get_dataframe' instead of a list? The other direction could be accommodated too (old lyse clients talking to new lyse servers) by using a more complex if statement on the server side (to capture both 'get_dataframe' and ['get_dataframe', ..]. So you get an (unhelpful) error message if you use new client with old server and take advantage of the new kwargs (although could we catch the error returned by the old ZMQ server and present a more useful error to the user?)

Thoughts?

Regarding the RangeIndex, there is a comment about falling back to RangeIndex just below the call to zmq_get in data(). I'm not familiar with the piece of code, but just noticed it when looking at the other stuff now. Might be worth seeing if the try/except is suppressing some useful debug information there.

[philipstarkey]

Ok I think I see the issue actually. The app.filebox.shots_model.dataframe uses a RangeIndex, then when the dataframe reaches lyse.data() its changed to a multiindex. Running filter() in WebServer.handler runs it on app.filebox.shots_model.dataframe and can end up filtering out the columns that would later be used to generate the multiindex. Then lyse.data() swallows the error generated when trying to make the multiindex and just returns the dataframe with the rangeindex.

I guess the solution is to move the multiindex-generating code from lyse.data() to WebServer.handler()? That way it could be run before filter().

Missed this while writing my own reply. Yeah, that could be a good idea. It does seem like filter should be run after generating the multiindex. Might need to test how this interacts with v3.0 server taking to v3.1 client though (and 3.1 server talking to 3.0 client). The code to convert to multi-index may need to be in both client and server until 4.0?

[zakv]

Was the use of shlex etc. to maintain backwards/forwards compatibility when accessing the dataframe from a standalone script? (For example lyse v3.0 server talking to lyse v3.1 client on another PC?).

Nope, somehow the idea that I could send anything besides a string didn't occur to me. That's a much better idea so I'll switch it over to do that instead and get rid of the superfluous imports.

Normally I would have probably just sent something like ['get_dataframe', n_sequences, filter_kwargs] as the command passed to zmq_get (zmq_get can send python objects), but I guess that would mean new lyse clients would raise an error when talking to old lyse servers. Oh...if n_sequences is None and filter_kwargs is None, the new client could fall back to sending just 'get_dataframe' instead of a list? The other direction could be accommodated too (old lyse clients talking to new lyse servers) by using a more complex if statement on the server side (to capture both 'get_dataframe' and ['get_dataframe', ..]. So you get an (unhelpful) error message if you use new client with old server and take advantage of the new kwargs (although could we catch the error returned by the old ZMQ server and present a more useful error to the user?)

Yeah that is a tricky situation which I hadn't considered. I can't think of a better solution than what you've suggested here. Actually the old lyse server just ignores the request if command is a list, so zmq_get() returns None in that case. That makes it easy to notice and raise a helpful error client-side if the server is outdated and doesn't support the new arguments. It can also fall back to sending just 'get dataframe' if n_sequences and filter_kwargs are None.

It does seem like filter should be run after generating the multiindex. Might need to test how this interacts with v3.0 server taking to v3.1 client though (and 3.1 server talking to 3.0 client). The code to convert to multi-index may need to be in both client and server until 4.0?

Yep I think you're right.

Edit: Actually I was mistaken, the old version would return a string starting with 'error' if an unrecognized command was sent. Still easy to catch though.

[zakv]

Looks like some PRs got merged into master. Should I do a rebase / force push for this branch? Or is the plan to just do a merge?

[philipstarkey]

Looks like some PRs got merged into master. Should I do a rebase / force push for this branch? Or is the plan to just do a merge?

I don't think you need to rebase unless you want to test the docs build. The rebase will probably also stuff up the git commit references in your comments, so I'd probably lean towards just merging and then patching any issues with the docs separately.