nirfsg: Enable complex number support for gRPC and update system tests#2172
nirfsg: Enable complex number support for gRPC and update system tests#2172
Conversation
rahur-NI
requested review from
marcoskirsch,
ni-jfitzger and
vnktshr21
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2172 +/- ##
==========================================
+ Coverage 88.76% 89.80% +1.04%
==========================================
Files 73 73
Lines 18986 19004 +18
==========================================
+ Hits 16853 17067 +214
+ Misses 2133 1937 -196
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
build/templates/_grpc_stub_interpreter.py/numpy_read_method.py.mako
Outdated
Show resolved
Hide resolved
build/templates/_grpc_stub_interpreter.py/numpy_read_method.py.mako
Outdated
Show resolved
Hide resolved
| % if included_in_proto: | ||
| % if numpy_complex_params: | ||
| % for p in numpy_complex_params: |
There was a problem hiding this comment.
The nested conditionals, loops, etc make the code hard to read when you don't indent them.
See build\templates_library_interpreter.py\numpy_read_method.py.mako for a cleaner example.
There was a problem hiding this comment.
I have updated Jay.
| <% | ||
| '''Renders a NotImplemented method for to the passed-in function metadata, because numpy is not supported over grpc.''' | ||
|
|
||
| '''Renders a GrpcStubInterpreter method for numpy write operations with complex number support.''' |
There was a problem hiding this comment.
I'm concerned about this comment, because the name of this file is actually numpy_read_method.py.mako. It's for read methods, but the write methods happen to also use it, at the moment, because they both currently just raise NotImplementedError.
It's probably time for _grpc_stub_interpreter.py/numpy_read_method.py.mako and _grpc_stub_interpreter.py\numpy_write_method.py.mako to diverge, just like the library_interpreter templates.
I'm especially concerned about the changes here because you're only using it for write methods, and it may lead to bad codegen, in the future, for read methods.
There was a problem hiding this comment.
Yes.. I agree to it. We can modify numpy_write_method.py.mako and keep the default changes in numpy_read_method.py.mako. I have updated the change
|
You should add new tests to src\nifake\unit_tests\test_grpc.py |
|
I'm worried about the system test results. We seem to have hung as soon as we reached the first nirfsg gRPC test for 32-bit. You may want to investigate on a local system. |
| def session_creation_kwargs(self): | ||
| return {} | ||
|
|
||
| def test_get_all_named_waveform_names(self, rfsg_device_session): |
There was a problem hiding this comment.
Can you add a comment about why this test was moved to TestLibrary and when it can be moved back?
| raise NotImplementedError('numpy-specific methods are not supported over gRPC') | ||
| waveform_data_array_list = [ | ||
| grpc_complex_types.NIComplexNumberF32(real=val.real, imaginary=val.imag) | ||
| for val in waveform_data_array.ravel() |
There was a problem hiding this comment.
Is it just my imagination or is ravel not actually changing anything except for in create_deembedding_sparameter_table_array()? If so, maybe add a comment about this to the template.
| sparameter_table_list = [ | ||
| grpc_complex_types.NIComplexNumber(real=val.real, imaginary=val.imag) | ||
| for val in sparameter_table.ravel() | ||
| ] | ||
| self._invoke( | ||
| self._client.CreateDeembeddingSparameterTableArray, | ||
| grpc_types.CreateDeembeddingSparameterTableArrayRequest(vi=self._vi, port=port, table_name=table_name, frequencies=frequencies, sparameter_table=sparameter_table_list, number_of_ports=number_of_ports, sparameter_orientation_raw=sparameter_orientation.value), | ||
| ) |
There was a problem hiding this comment.
I have no idea if this or the other functions are correct. I'm going to lean heavily on the testing you've done and the review of @vnktshr21 , rather than wearing out my brain trying to work through it, when I don't even know what the gRPC code expects.
4 participants
I've updated CHANGELOG.md if applicable.What does this Pull Request accomplish?
numpy_read_method.py.makotemplate with snippets copied over fromdefault_method.py.makoand also included gRPC generation support for numpy complex methods._grpc_stub_interpreter.py.makotemplate to importnidevice_pb2 as grpc_complex_typesonly if complex numbers are used in the module.grpc_server_config.jsonfile for nirfsg to include the address and port for server listening.List issues fixed by this Pull Request below, if any.
NA
What testing has been done?
TestGrpcclass intest_system_nirfsg.pyto include gRPC-based system test session setup.test_get_all_named_waveform_namestest will be only passing when the next release version of grpc-device server is used for nimi-python system tests. So this test case was moved from common path to TestLibrary class intest_system_nirfsg.pyfile to skip being tested against grpc. This movement will be reverted once we update the grpc-device version being used later.use_simulated_sessionoption as False, all the nirfsg test cases are passing as expected.