search

chrisjbillington / zprocess

A collection of utilities for multiprocessing using zeromq.
BSD 2-Clause "Simplified" License
2 stars 5 forks source link

Error being thrown when updating from zprocess 2.12.5 to 2.13.0 #11

Closed chrisjbillington closed 5 years ago

chrisjbillington commented 5 years ago

Original report by Lars Kohfahl (Bitbucket: 5aafc5d11396802a57aa7f3b, ).

I tried to update today some of the labscript updates from the past two week. Including the labscript_utils fixes for remote workers (https://bitbucket.org/labscript_suite/labscript_utils/commits/e1db05c9e0e4be051a6715d305d05baf5a7e5678). I thereby updated to the newest zprocess and then got the following error when closing BLACS (everthing else seems to work fine).

Looking into the Blacs Log showed this:

Installing different versions of zprocess revealed, that the problem arises when updating from 2.12.5 to 2.13.0.

chrisjbillington commented 5 years ago

Original comment by Jan Werkmann (Bitbucket: 557058:a70cc9cf-684e-4849-a61a-9ade4d7218b5, GitHub: PhyNerd).

Seems to be our Analog In Broker that doesn’t play well with the update. The Code can be found in PR: https://bitbucket.org/labscript_suite/blacs/pull-requests/19

I’ll check this later

chrisjbillington commented 5 years ago

Original comment by Chris Billington (Bitbucket: 557058:cbf1bc43-1dc2-477b-9e25-1a8f40fd7ee3, GitHub: cbillington).

In case it's relevant, zprocess 2.13.0 had a major bug that broke it on Python 2. So on the offchance you are on Python 2, I suggest updating zprocess a bit further. If you're on Pytohn 3 though (which I'm pretty sure you are) this shouldn't be relevant.

chrisjbillington commented 5 years ago

Original comment by Chris Billington (Bitbucket: 557058:cbf1bc43-1dc2-477b-9e25-1a8f40fd7ee3, GitHub: cbillington).

I've had a similar report from someone else, so I might be able to reproduce it here. They say it occurs if they leave BLACS open overnight though, so we have to wait :).

If the issue reliably occurs every time for you, if you could figure out which commit between 2.12.5 and 2.13.0 broke things, that would probably help.

List of commits are here, there are ten between the versions:

https://bitbucket.org/cbillington/zprocess/commits/all

You can install a specific commit by doing pip uninstall zprocess followed by pip install hg+https://bitbucket.org/cbillington/zprocess/@6206fb1, replacing 6206fb1 with the specific hash of the commit.

I apologise if a change in zprocess broke your AI broker. zprocess is not supposed to be breaking compatibility, so if it did this is my fault, not yours.

Also, I haven't forgotten about the manual mode AI pull requests and will get to them eventually, for what it's worth!

chrisjbillington commented 5 years ago

Original comment by Jan Werkmann (Bitbucket: 557058:a70cc9cf-684e-4849-a61a-9ade4d7218b5, GitHub: PhyNerd).

I found the issue it is not a bug more a misuse of zprocess by me.

In commit 139dcc6 you introduce a message back to the parent process that says terminated. I used the from_child.get() function to forward any exceptions from the broker process => exception is raised in BLACS with message ‘terminated’.

‌

Solution is replacing(blacs/__main__.py):

    def _check_broker(from_child):
        exception = format(from_child.get())
        raise(Exception(exception))

with:

    def _check_broker(from_child):
        exception = format(from_child.get())
        if exception != 'terminated':
          raise(Exception(exception))
chrisjbillington commented 5 years ago

Original comment by Jan Werkmann (Bitbucket: 557058:a70cc9cf-684e-4849-a61a-9ade4d7218b5, GitHub: PhyNerd).

I’ll also update the pull request

chrisjbillington commented 5 years ago

Original comment by Chris Billington (Bitbucket: 557058:cbf1bc43-1dc2-477b-9e25-1a8f40fd7ee3, GitHub: cbillington).

Thanks for debugging! It would have taken me a while to find the cause of this otherwise.

And I appreciate your willingness to take the blame, but I disagree, zprocess should not be returning a special message to code that does not expect it!

The addition of this message was important to resolve a potentially long hang at process startup if the user terminated the child process before it managed to connect to the parent, discussed here. But its use should be limited to that context and not returned to user code later on if the subprocess is terminated. I've resolved this in af85e33 and after a little testing to make sure it works as expected will release as zprocess 2.13.3.

No need to change your broker code back, it's harmless, plus I will be going over it in any case when putting more effort into merging it.

chrisjbillington commented 5 years ago

Original comment by Chris Billington (Bitbucket: 557058:cbf1bc43-1dc2-477b-9e25-1a8f40fd7ee3, GitHub: cbillington).

This is fixed in zprocess 2.13.3

Thanks for the bug report! Even though your lab's specific issue was resolved separately, this same bug was crashing BLACS if the user clicked to restart a tab with unlucky timing, so it was good to find out about it and fix it.

chrisjbillington commented 5 years ago

Original comment by Chris Billington (Bitbucket: 557058:cbf1bc43-1dc2-477b-9e25-1a8f40fd7ee3, GitHub: cbillington).

Fixed in 2.13.3