Conversation
…the class constants marked with `_rpc_constants = ["constant_a", "constant_b", ...]` can be changed by calling `class_rpc_proxy_inst.constant_a(new_value)`. The new value must be of same type and length as the original value to be able to change the value. This means that e.g. `None` cannot be used to set the RPC constant as 'optional', and the constant cannot have union of types either. Need to edit the documentation about this still.
…thout a new value to return the current value.
…t value calls to be settables and gettables instead, so that they can be modified like any class attribute.
…bout using RPC constants.
… constant in help.
…QuTech-Delft/QMI into changing-rpc-constants-on-proxy
…ts definition in serial device transport string docstring in `create_transport`.
…_rpc_constants names so that no method names nor QMI signal names etc. can (accidentally) be named as _rpc_constants. But it makes several unittests to fail now.
…stants` to be a `set` type. Added more robust checking of the RPC constants so that giving any method, property, class name or QMI signal is not allowed. Boy-scouted a bug in `datastore.py`.
… Properties" everywhere where applicable.
…stants` about it being deprecated.
…s-on-proxy # Conflicts: # CHANGELOG.md # qmi/core/rpc.py # qmi/data/dataset.py
… limiting coverage version in pyproject.toml to 7.13.5.
… limiting coverage version in pyproject.toml to 7.2~
… limiting coverage version in pyproject.toml to 7.2~
… limiting coverage version in pyproject.toml to 7.14.1
… removing .coveragerc file
… setting to version 7.9.2.
…on limits and now setting Python versions to 3.13.13 and 3.14.5. Also placed back `.coveragerc` file
…ers. setting back to use latest Python versions.
…unittests to see if those also hang - i.e. is the problem with coverage
…only on test_rpc.TestRPC.test_blocking_rpc only, as that seems to be the test that hangs.
… that the traceback is first turned into a tuple of strings before sending it to the RpcFuture, not after.
…n about the RPC properties and changed one exception type in the code. Added a unit-test to check this exception.
…est by using deterministic set of values instead of random values.
…ties in high finesse and montana drivers.
…ine type in hdf5recorder.py
…r since upgrade of NumPy dependency to > V2.0. The `repr(val)` for numpy floats and ints return now also the `np.floatxx` and `np.intyy` as part of the repr string. This causes a breakdown in compiling. Replacing the `repr` with `str` solves this issue and should not change any other behaviour.
bvanommen
left a comment
bvanommen
left a comment
There was a problem hiding this comment.
That should be everything. I'll admit I didn't look at all the new tests in 100% detail. Most important is the tb stuff
| # Received result from RPC call. | ||
| # Received result from RPC method call. | ||
| self._set_result(message.state, message.result) | ||
| elif isinstance(message, QMI_PropertyRpcReplyMessage): |
There was a problem hiding this comment.
I thought of that originally, but I want to have it clear that we will handle both cases, with the comment line after the (el)if.
| self._set_result( | ||
| QMI_RpcFutureState.RESULT_IS_EXCEPTION, (exc, | ||
| tuple(traceback.format_list(traceback.extract_tb(exc.__traceback__))) | ||
| )) |
There was a problem hiding this comment.
I have seen in the lab that this error providing may create incompatibility with servers using older QMI versions. Do we care?
There was a problem hiding this comment.
So, this error would come if an older QMI version tries to get a message/response from this newer QMI version RPC proxy, and it finishes in an exception. But not the other way around. I guess this is inevitable if we want to make this change, and not break the compatibility both ways? I didn't see a way to do it robustly both ways, at least.
| value, | ||
| set_property_value=True | ||
| ) | ||
| return |
There was a problem hiding this comment.
how does the caller know whether the call was a succes or failure?
There was a problem hiding this comment.
I think this relates to your comment in line 531. If we also add the traceback there, then the user should get the traceback if this fails. Otherwise, it would probably just fail silently.
| self._context.send_message(request) | ||
|
|
||
| except QMI_MessageDeliveryException as exc: | ||
| self._set_result(QMI_RpcFutureState.RESULT_IS_EXCEPTION, exc) |
There was a problem hiding this comment.
I think I understand, but to double check: we don't expect a tuple of exc and traceback here because send_message just raises message delivery exceptions?
There was a problem hiding this comment.
Now I'm not sure anymore. Doesn't this exception still end up in the wait() function? I guess there it gets handled under the "older QMI version" code path
There was a problem hiding this comment.
This should probably be handled like the other QMI_MessageDeliveryExceptions. I will change this to be the same.
| traceback.extract_tb(exc.__trackback__))) | ||
| self._set_result( | ||
| QMI_RpcFutureState.RESULT_IS_EXCEPTION, (exc, | ||
| tuple(traceback.format_list(traceback.extract_tb(exc.__traceback__))) |
There was a problem hiding this comment.
But then here we do expect a tuple. I guess we shouldn't do so here either
There was a problem hiding this comment.
This one is the correct one.
| property_name: str, | ||
| rpc_lock_token: QMI_LockTokenDescriptor | None, | ||
| property_value: Any = None, | ||
| *, |
There was a problem hiding this comment.
why use this star here so set_property_value cannot be positional?
There was a problem hiding this comment.
Just for future proofing that the any other calls (accidentally?) made to rpc_property_call will not start setting the property values without explicitly writing out the keyword input. Also some more reasoning in https://peps.python.org/pep-3102/.
| else: | ||
| raise AttributeError("Not allowed to set new attributes on a proxy class.") | ||
|
|
||
| object.__setattr__(self, name, value) |
There was a problem hiding this comment.
I'm not so sure when this fallback setattr will get called. When the object is not initialized? Which situation would that be
There was a problem hiding this comment.
Yes. Although, that should not be the case probably ever... It's just more like a fallback now in case of irregular use.
| except BaseException as exception: | ||
| _logger.debug("Modifying RPC property failed", exc_info=True) | ||
| result_type = QMI_RpcFutureState.RESULT_IS_EXCEPTION | ||
| result = exception |
There was a problem hiding this comment.
I guess for consistency it would better to now send the traceback along in a tuple like we do for method calls
There was a problem hiding this comment.
But here the error can be logged now if the QMI_DEBUG is set, together with exc_info. And this goes to the _RpcThread and not to the QMI_RpcFuture class.
| # Check that the property was marked as RPC property. | ||
| if not hasattr(self._rpc_object, request.property_name): | ||
| raise QMI_UnknownRpcException( | ||
| f"Object {request.destination_address.object_id} of type {type(self._rpc_object).__name__}" +\ |
There was a problem hiding this comment.
Maybe the error message could explicitly state that it's not an RPC property? Likely a common error will be that you forget to set the property as RPC-able
There was a problem hiding this comment.
On that note, you would somehow want to decorate class variables like we do methods. You could make a function RPC_Variable(var: Any) so that you declare variables as RPC_Variable(var_1) in the class. But I guess we'll never have that many RPC properties and they're all together at the start of the class declaration
There was a problem hiding this comment.
I will add in the text "RPC property" instead of simply "property". That is probably sufficient?
About the decoratable "RPC_Variable"s. Something in that direction is thought of in the project board under epics. Have a look ;) These "RPC properties" here are OK for these simpler class-level variables | attributes..
| except ValueError: | ||
| return False | ||
| return True | ||
|
|
There was a problem hiding this comment.
something like a python ndarray will not have its elements checked here, I think. Somehow I guess you want to make sure that any other structure that can be indexed an does not fall under the checked types here should return False instead of True
There was a problem hiding this comment.
Ah I guess something like a string would be an exception, there length change is fine
There was a problem hiding this comment.
Indeed, numpy arrays are not considered here element-wise. It is possible that someone might want to use a numpy array of course. Perhaps we can leave it for a possible future addition and not do it now?
…fter review comments.
2 participants
Related to #213.
With this, the "RPC constants" that are class constants on a RPC object, e.g. QMI_Instrument, have been renamed to "RPC properties, and can now be modified so that the modifications propagate back to the actual object, and not proxy only. With this, the proxy class of the object was also made prohibitive on adding new class attributes, RPC properties can only be modified with same type (and size if applicable) values, and also class constants not defined with
_rpc_propertiesare not modifiable and will lead to anAttributeError.Further checks are made that the given
_rpc_propertiesvalues are not referring to e.g. a method, QMI signal attribute, property, etc. Invalid naming will raise aQMI_UsageError._rpc_constantsuse will now give a deprecation warning, and will be handled the same way as the_rpc_properties.Some tutorial examples were also added to illustrate this new functionality.