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
❌ Your patch status has failed because the patch coverage (50.00%) 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 #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.
| <% | ||
| '''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.
|
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.