Proxy Instruments#46
Conversation
…ct in the testing notebook. Did some modification to base driver
…struments # Conflicts: # src/pqnstack/base/errors.py
# Conflicts: # src/pqnstack/base/errors.py # src/pqnstack/network/client.py # src/pqnstack/network/node.py # src/pqnstack/pqn/drivers/rotator.py
|
@Benjamin-Nussbaum @SoroushHoseini I have just pushed the mypy and ruff clean up so this PR is ready for a review. I expect some pushback on some of the things here, we can discuss this in the software meeting if this is too little time for a proper review beforehand since it is also a big PR |
# Conflicts: # src/pqnstack/base/driver.py # src/pqnstack/base/errors.py # src/pqnstack/network/client.py # src/pqnstack/network/node.py # tests/messaging/client.py # tests/messaging/node.py
|
|
||
| class InstrumentClient(ClientBase): | ||
| def __init__( | ||
| self, name: str, host: str, port: int | str, router_name: str, instrument_name: str, node_name: str |
There was a problem hiding this comment.
Standardize type of port to one of int or str, not a union of both. I suggest int since the port is always just a number. (Python stdlib also uses int for ports)
| def trigger_operation(self, operation: str, *args, **kwargs) -> Any: | ||
| packet = Packet( | ||
| intent=PacketIntent.CONTROL, | ||
| request=str(self.instrument_name) + ":OPERATION:" + operation, |
There was a problem hiding this comment.
instrument_name is supplied as a str in the constructor - the explicit str() call seems unnecessary.
| def trigger_parameter(self, parameter: str, *args, **kwargs) -> Any: | ||
| packet = Packet( | ||
| intent=PacketIntent.CONTROL, | ||
| request=str(self.instrument_name) + ":PARAMETER:" + parameter, | ||
| source=self.name, | ||
| destination=self.node_name, | ||
| payload=(args, kwargs), | ||
| ) | ||
| response = self.ask(packet) | ||
| if response is None: | ||
| msg = "No response received." | ||
| raise PacketError(msg) | ||
|
|
||
| return response.payload |
There was a problem hiding this comment.
Quite similar to the operation code - one route to fix would be separating creation/use of the Packets.
| response = self.ask(packet) | ||
| if response is None: | ||
| msg = "No response received." | ||
| raise PacketError(msg) |
There was a problem hiding this comment.
Would it be a bad idea to put this None check inside ask()?
There was a problem hiding this comment.
I did this now, the only change that this means is that when a timeout error happens it is raised instead of returning None. Now those checks are not necessary anymore
| class ProxyInstrument(DeviceDriver): | ||
| """The address here is the zmq address of the router that the InstrumentClient will talk to.""" | ||
|
|
||
| DEVICE_CLASS = DeviceClass.PROXY | ||
|
|
||
| def __init__( | ||
| self, | ||
| name: str, | ||
| desc: str, | ||
| address: str, | ||
| host: str, | ||
| port: str, | ||
| node_name: str, | ||
| router_name: str, | ||
| parameters: set[str], | ||
| operations: dict[str, Callable], | ||
| ) -> None: |
There was a problem hiding this comment.
Seems like a lot of arguments to initialize a ProxyInstrument - are they all needed?
There was a problem hiding this comment.
They kinda are. I can merge host and port but then I would need to separate them by code for the client. Same with parameters and operations, I already have those when I ask the server for them, I don't need to have them there but they would add an extra call to the server.
If those things are ok, I can implement those changes
| return Packet( | ||
| intent=PacketIntent.ERROR, | ||
| request="ERROR", | ||
| source=self.name, | ||
| destination=packet.source, | ||
| payload=f"Instrument '{packet.payload}' not found.", | ||
| ) |
There was a problem hiding this comment.
It might be a good idea to create a wrapper function that returns error Packets so only the destination and payload are needed here.
There was a problem hiding this comment.
Lots of logic handling instrument control - compacting the Packet creation will help make it more digestible, but it might also be worth separating out the logic to handle each request_type in its own function.
# Conflicts: # src/pqnstack/network/client.py # src/pqnstack/network/node.py
Benjamin-Nussbaum
force-pushed
the
feature/proxy-instruments
branch
from
8d258b9 to
95d4693
Compare
4 participants
Opening the PR as draft since I am encountering some mypy issues that will require a little more syntax noodling before the code is ready, the big picture idea and structure should not change however.
The idea of a Proxy instrument is that when using a remote resource in the network, a client can simply ask for a device from a node and a ProxyInstrument is created on the client side. This ProxyInstrument should behave as if the instrument is running locally to the user but in the background it asks whatever router that client was talking to send CONTROL packets to a node that then executes them.
To test it again simply run the router.py, node.py, and client.py in that order in your computer.
I have tested it with the rotator working through the PI and I was able to control the pi without having to ssh into it.
I am not convinced that the ProxyInstrument and InstrumentClient should live in the same client.py but I was having some import errors so I left them there for now.