Closed GDYendell closed 1 year ago
@ajgdls are you happy to drop 01027f8 from these changes because #300 fixes the problem?
@ajgdls are you happy to drop 01027f8 from these changes because #300 fixes the problem?
Yes agreed this can be dropped.
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.
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.
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.
See #307
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?
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).
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.
@timcnicholls I have updated this to remove usage of zmq strtypes and reimplement our own versions.
A few additions coming out of adding support for the Xspress detector