Xspress detector developments

[GDYendell]

A few additions coming out of adding support for the Xspress detector

• Waiting on Update packaging #301
• Waiting on Add git URL for odin-control requirement #307

[GDYendell]

@ajgdls are you happy to drop 01027f8 from these changes because #300 fixes the problem?

[ajgdls]

@ajgdls are you happy to drop 01027f8 from these changes because #300 fixes the problem?

Yes agreed this can be dropped.

[GDYendell]

Check for and update minimum pyzmq version for cast_{bytes,unicode}.

pyzmq 23.0 adds deprecation warnings for strtypes, so maybe these casts are made redundant by other changes. Investigate and see if we can remove these completely before updating to >=23.0.

[timcnicholls]

Check for and update minimum pyzmq version for cast_{bytes,unicode}.

pyzmq 23.0 adds deprecation warnings for strtypes, so maybe these casts are made redundant by other changes. Investigate and see if we can remove these completely before updating to >=23.0.

@GDYendell I'm looking into this now. While testing this branch with a fresh venv and pyzmq install it no longer seems possible to do a pip install -e ., as setup.cfg now expresses a dependency on odin-control, which doesn't resolve to a published PyPI package. It didn't use to be necessary to have odin-control for this package - is it now? If so, the dependency needs updating to use a Github URL rather than a simple package name.

[GDYendell]

Ah sorry about that @timcnicholls. I missed that because it works at DLS with our internal PyPI mirror. I think I did have use to have to manually install odin-control in a venv before odin-data, but that may have just been a DLS thing.

We can add the github URL to setup.cfg - where was this defined before? It isn't in the old setup.py, setup.cfg or requirements*.txt.

[GDYendell]

See #307

[timcnicholls]

We can add the github URL to setup.cfg - where was this defined before? It isn't in the old setup.py, setup.cfg or requirements*.txt.

I think it relied on manual installation of odin-control before installing?

[timcnicholls]

OK, I've investigated this a bit more, using pyzmq version 25 although it's applicable for >23 . The deprecation of the strtypes cast_xxx methods is not because the translation from strings to bytes is automatic, rather that these are not used internally any more. The send_string and recv_string convenience methods on the ZMQ socket class use internal serialize/deserlalize helpers. There are also no equivalent multipart send/recv methods for strings.

I think the conclusion is that we need to reimplement/retain our own cast_xxx helper functions in IpcChannel, but this is worth a discussion first. Generally we only use IpcChannel to send JSON objects as strings, so could switch to using the ZMQ socket send_string and recv_string (or even the JSON versions) but there are cases where the channels are used with raw data (e.g. in live view).

[GDYendell]

We should probably mirror the pyzmq library and have IPCChannel.send and IPCChannel.send_str, but that would be a breaking change and cause exceptions in existing client code that calls IPCChannel.send with str. As a transition we could update IPCChannel.send to remove the casting and instead just type check and call Socket.send / Socket.send_str as appropriate - then at least ipc_*_channel won't be using the deprecated API.

I don't see an obvious way of updating IPCChannel.recv without breaking client code, though, because we would have to make a choice as to whether to return str or bytes. Currently it always does a Socket.recv_multipart and casts to str. It looks like the live view data currently gets cast to str and it then calls odin.util.convert_unicode_to_string on part and passes another to np.fromstring which all seems unnecessary.

Note odin.util.convert_unicode_to_string is used exclusively for tests except for the above instance. There is also a copy of the function in odin_data that is never used.

I also think adding type hints will make this much clearer, but we have to drop python 2 for that.

[GDYendell]

@timcnicholls I have updated this to remove usage of zmq strtypes and reimplement our own versions.