Add mechanism to allow subscribing to Driver Warning events #2099
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (68.18%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2099 +/- ##
==========================================
- Coverage 91.34% 91.17% -0.17%
==========================================
Files 66 66
Lines 16292 16382 +90
==========================================
+ Hits 14882 14937 +55
- Misses 1410 1445 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| @@ -112,6 +113,14 @@ class LibraryInterpreter(object): | |||
| def get_session_handle(self): | |||
| return self._${config['session_handle_parameter_name']} | |||
|
|
|||
| def generate_driver_warning_event(self, driverwarning: errors.DriverWarning): | |||
There was a problem hiding this comment.
Missing an underscore
driverwarning -> driver_warning
|
|
||
| Returns: | ||
| session (nidcpower.Session): A session object representing the device. | ||
|
|
||
| ''' | ||
| driver_warning_event = errors.DriverWarningEvent() |
There was a problem hiding this comment.
From what I can tell, this doesn't actually allow users to subscribe to these events.
The Session object is unconditionally creating the errors.DriverWarningEvent().
Users can't access it without breaking encapsulation. Even then, not easily. It's a private member of the interpreter object, which is, itself, a private member of the Session object.
There was a problem hiding this comment.
Naming might be the culprit here. errors.DriverWarningEvent() is not the event itself, but a class which allows handling of warnings as events. I've handled it somewhat similar to session_handle which gets created during session initialize at session object and then gets passed into library_interpreter for future use. The same instance of DriverWarningEvent() object is at play at the session object(public member) and library interpreter(private member), somewhat like a shared_ptr. Hence the addition of generate_warning_event method in library interpreter to access its private member. Encapsulation is not broken as such here as per my interpretation.
Also, the functionality of this is tested already and I've added a unit test in nifake to check the subscription and notification operation.
I see interpreter member name in session object not starting with _. Maybe there was a need to not treat interpreter as private?
|
Asynchronous handling is mentioned in the linked issue. Is that important, because I don't see how this provides asynchronous handling. |
| @@ -120,6 +120,28 @@ class SelfTestError(Error): | |||
|
|
|||
|
|
|||
| % endif | |||
| class DriverWarningEvent: | |||
There was a problem hiding this comment.
- Shouldn't this class have the word "handler" in its name? It doesn't represent an event, it represents a class that handles warnings.
- Does the word "Event" add value? Is it some Python convention that there are "warning events" or can we just say "warning"?
- Is this class something that customers interact with or is it internal?
There was a problem hiding this comment.
The driver_warning_event member of session object will be of this class type. Thought process behind the name was that since user is going to call subscribe on this member, driver_warning_event.subscribe(callback_function) would look more apt than driver_warning_event_handler.subscribe(callback_function). The class essentially provides methods for handling of warnings as events, but its the handle_error method which actually handles warnings overall.
DriverWarning is already being used as the name for the class which houses the details of warning thrown by the driver(inherits native python Warning class). I guess "Event" here is only conveying that the action is going to be taken only in the event of a driver warning.
Maybe we can have the name of the class as DriverWarningEventHandler and the member instance in the session object can still have the name driver_warning_event?
| def __init__(self): | ||
| self.subscribers = [] | ||
|
|
||
| def subscribe(self, callback): |
There was a problem hiding this comment.
I'm guessing callback is not a callback, but rather a function? name should be descriptive and accurate.
There was a problem hiding this comment.
Makes sense. Will change this to say function_to_callback instead.
| def subscribe(self, callback): | ||
| """Subscribe to warning events.""" | ||
| if callback not in self.subscribers: | ||
| self.subscribers.append(callback) |
There was a problem hiding this comment.
do you support subscribing the same thing N times? what is the use case?
if not: you should raise and have unit test coverage for the logic
There was a problem hiding this comment.
Current implementation does not restrict the users to have just one callback function. Went with this approach mostly to match the RFSA/G .NET API behavior. I do not see a reason for restricting as such, however I do not have a known use case for this either. This could enable handling different warnings in their own way by defining different callbacks and registering all of them. Gives a flexibility that potentially can lead to a cleaner user application.
| if callback not in self.subscribers: | ||
| self.subscribers.append(callback) | ||
|
|
||
| def unsubscribe(self, callback): |
There was a problem hiding this comment.
is it ok to unsubscribe a callback function that was not subscribed?
If yes, make sure that's covered in unit tests with the proper behavior.
There was a problem hiding this comment.
In the current implementation, yes it is ok for the users to do this. Thought process was to not be strict on this regard since the result of unsubscription or not being notified again occurs anyways.
Will add a unit test for this behavior.
| @@ -120,6 +120,28 @@ class SelfTestError(Error): | |||
|
|
|||
|
|
|||
| % endif | |||
| class DriverWarningEvent: | |||
There was a problem hiding this comment.
Did you guys invent this whole thing from scratch or did you derive from another NI driver or something else?
I'm wondering about terminology, etc. but not if it introduces inconsistency across our drivers or even Python conventions.
There was a problem hiding this comment.
The idea of warning event is derived from RFSG and RFSA drivers' .NET API experience (explained here). Implementation was just based on expected behavior. I do not have a single reference as such from other python modules, but it was sort of the approach taken generally wherever folks have implemented a event handler in python.
| @@ -245,17 +245,34 @@ if grpc_supported: | |||
| 'use_in_python_api': False, | |||
| }, | |||
| ) | |||
|
|
|||
| ctor_for_docs['parameters'].append( | |||
There was a problem hiding this comment.
injecting metadata in a mako template seems very hacky
@ni-jfitzger do we have prior art for something like this?
There was a problem hiding this comment.
@ni-jfitzger do we have prior art for something like this?
Unfortunately, we do.
It's just a few lines up.
nimi-python/build/templates/session.py.mako
Line 233 in e66d8ee
I agree. It's hacky. It should only be done if it's not possible to include directly in our metadata.
I consider the "prior art" to be tech debt.
There was a problem hiding this comment.
This is here since my initial approach was to try to restrict this only for rfsg. This was conditional inject at that point. Given the approach now is to apply for all drivers, I could move this to config metadata and consume from there.
| { | ||
| 'default_value': None, | ||
| 'direction': 'in', | ||
| 'documentation': { 'description': 'Driver warning event which can be subscribed to, with a callback method.\nSample callback method:\n\ndef sample_callback_method(driver_warning: ' + module_name + '.DriverWarning):\n print(str(driver_warning))\n' }, |
There was a problem hiding this comment.
I didn't find this description to be very clear or helpful. Once we nail the class terminology we need to workshop this.
| def generate_driver_warning_event(self, driverwarning: errors.DriverWarning): | ||
| '''generate_driver_warning_event | ||
|
|
||
| Generates a driver warning event. |
There was a problem hiding this comment.
What does it mean to "generate a driver warning event"?
Are you trying to say this is a function that should be called whenever the driver returns a warning?
What calls this function (is it internal only or is it called by clients of this class)? In other words should this be prefixed by an underscore?
There was a problem hiding this comment.
We call this function from handle_error function in our errors.py(similar to get_error_description placed next to this method). Hence the lack of underscore. This should be public method of library interpreter which owns the warning_event_handler and has interaction with a utility function in errors.py. Completely internal though. Idea was to not expose warning event handler's internal operations in session object. Library interpreter object owning the current session's warning_event_handler instance was making sense since there should be just one handler per session.
| @@ -101,6 +101,28 @@ def __init__(self, code, msg): | |||
| super(SelfTestError, self).__init__('Self-test failed with code {}: {}'.format(code, msg)) | |||
|
|
|||
|
|
|||
| class DriverWarningEvent: | |||
There was a problem hiding this comment.
@bkeryan Hey Brad, question for you:
As of now, nimi-python drivers just log warnings. This PR adds a way for clients to pass a callback function for whenever a warning occurs so that clients can customize the behavior.
Do you know what other driver Python APIs do with warnings?
Does this seem like a mechanism worth standardizing across the NI driver Python APIs so that customers benefit from the consistency?
I don't want to invent something only to have nisyscfg or nidaqmx invent something different for the sake of being different.
There was a problem hiding this comment.
@bkeryan , any thoughts on this?
There was a problem hiding this comment.
As of now, nimi-python drivers just log warnings.
I think using the standard Python warnings module is more flexible than just logging. It also allows applications to catch warnings or treat them as errors, and testing tools like pytest show warnings more prominently than logging.
Do you know what other driver Python APIs do with warnings?
nidaqmx reports them via the standard Python warnings module.
The DAQmx .NET API uses an event like this, though.
Does this seem like a mechanism worth standardizing across the NI driver Python APIs so that customers benefit from the consistency?
I don't know. I think #2088 is the only request I've seen for this capability in Python.
| setattr(self, f, MagicMock(spec_set=getattr(self, f), side_effect=_mock_helper.MockFunctionCallError(f))) | ||
|
|
||
| def setup_method(self, method): | ||
| self.patched_library_interpreter = self.PatchedLibraryInterpreter(None) | ||
| self.driver_warning_event = nifake.errors.DriverWarningEvent() |
There was a problem hiding this comment.
Is this the intent, that the client modifies the member directly if they want to customize the behavior for warnings?
There was a problem hiding this comment.
The client would just call subscribe function on the driver_warning_event member of session object by passing a callback function. There is no modification of member necessary. Do you see a need to restrict users from overwriting the driver_warning_event member itself?
Thats a good point. In the current approach, we will execute all the registered callback functions before the API function execution returns. We will need to spawn asynchronous threads for executing callback functions to get the asynchronous behavior. I will check on the requirement for |
| @@ -101,6 +101,28 @@ def __init__(self, code, msg): | |||
| super(SelfTestError, self).__init__('Self-test failed with code {}: {}'.format(code, msg)) | |||
|
|
|||
|
|
|||
| class DriverWarningEvent: | |||
There was a problem hiding this comment.
As of now, nimi-python drivers just log warnings.
I think using the standard Python warnings module is more flexible than just logging. It also allows applications to catch warnings or treat them as errors, and testing tools like pytest show warnings more prominently than logging.
Do you know what other driver Python APIs do with warnings?
nidaqmx reports them via the standard Python warnings module.
The DAQmx .NET API uses an event like this, though.
Does this seem like a mechanism worth standardizing across the NI driver Python APIs so that customers benefit from the consistency?
I don't know. I think #2088 is the only request I've seen for this capability in Python.
There was a problem hiding this comment.
Comparison with other APIs:
NIDCPower .NET warnings:
- You use
session.DriverOperation.Warningto register/unregister warning event handlers - The event handler delegate takes
object senderas well as the warning event args. I think the sender is the session object, so you can call back into it.
DAQmx .NET warnings:
- You use the DaqSystem.Warning property to register/unregister warning event handlers. This is on a singleton object.
- You can use the DaqSystem.LastDaqWarning property to get the most recent warning and DaqSystem.ClearLastDaqWarning() to clear it. These are thread-local.
DAQmx Python events:
- The event callback should be passed the task object, except it passes you the underlying C or gRPC task handle instead. :( Event Callbacks Contain Unusable Parameters nidaqmx-python#143
- You can store the task reference in your callable (via a closure or callable object) so that it can call back into the task. However, there are some issues with calling task.close(): Can't close tasks in an event callback when using gRPC nidaqmx-python#559
There was a problem hiding this comment.
One missing feature is the ability to pass the session to the warning handler so it can call back into the session. You can work around this by using a different callable for each session.
Note that this creates a reference cycle: session -> interpreter -> warning_event_handler -> subscribers -> callable -> session
It's good if closing the session breaks the cycle, but apparently Python's GC can handle reference cycles.
Customers may want to use a weak ref to break the cycle. If the Session class has slots, it needs to include the __weakref__ slot.
|
|
||
| Returns: | ||
| session (nidcpower.Session): A session object representing the device. | ||
|
|
||
| ''' | ||
| driver_warning_event = errors.DriverWarningEvent() |
There was a problem hiding this comment.
This is not a member variable. It's a local variable in the __init__ method.
| @@ -7498,16 +7498,23 @@ def __init__(self, resource_name, channels=None, reset=False, options={}, indepe | |||
|
|
|||
| grpc_options (nidcpower.grpc_session_options.GrpcSessionOptions): MeasurementLink gRPC session options | |||
|
|
|||
| driver_warning_event (nidcpower.DriverWarningEvent): Driver warning event which can be subscribed to, with a callback method. | |||
There was a problem hiding this comment.
This is not the documentation for instance attributes. It's the documentation for the __init__ method arguments, but the variable is not actually an argument.
What does this Pull Request accomplish?
List issues fixed by this Pull Request below, if any.
What testing has been done?