Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement #1026

Blahpapto1 · 2024-11-29T14:16:53Z

This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

Adding Input Support for Measurment Client Generator and Service and updating Sample Measurement.

Why should this Pull Request be merged?

Adding Input Support for Client Generator, Service and Sample Measurement

What testing has been done?

Automated tests are passing after adding Double2DArray Input parameter.
Measurement is running fine through commandline.
Tested on MeasurementUI Editor with Sample Measurement.

Blahpapto1 · 2024-11-29T14:22:42Z

For parsing Double2DArray value we are converting the default_value to string in _support.py. Also, handling repr(value) conversions in get_configuration_and_output_metadata_by_index method. This is one of the approach that we think is the best way to handle these conversions.

Another way, we can handle the parsing of Double2DArray value is to handle it after render_template method (before black._format_str method). This creates the output template then we fix all the instance in the generated output template through regex. But this happens during the _create_file method call, which may or may not be the best place to handle this case from a design perspective.

github-actions · 2024-11-29T15:21:24Z

Test Results

40 files ±0 40 suites ±0 53m 12s ⏱️ + 2m 7s
703 tests ±0 703 ✅ ±0 0 💤 ±0 0 ❌ ±0
17 090 runs ±0 16 020 ✅ ±0 1 070 💤 ±0 0 ❌ ±0

Results for commit 1f5551c. ± Comparison against base commit 31c28b7.

♻️ This comment has been updated with latest results.

dixonjoel · 2024-12-04T15:38:14Z

packages/generator/ni_measurement_plugin_sdk_generator/client/_support.py

@@ -209,6 +212,18 @@ def get_configuration_and_output_metadata_by_index(
            default_value = default_value.value
        elif isinstance(default_value, list) and any(isinstance(e, Enum) for e in default_value):
            default_value = [e.value for e in default_value]
+        elif isinstance(default_value, str):
+            default_value = repr(default_value)
+        elif configuration_metadata[id].message_type == "ni.protobuf.types.Double2DArray":


Can you give an example of what 'default_value' is when it comes in here and what it looks like after you reconstruct it?

I think this approach is ok. I'd like to hear what @bkeryan or @mshafer-NI think.

I disagree. I think this approach is unacceptable. str(some_double_2d_array) is for display purposes and not for parsing.

_format_default_value should downcast the default value to a Double2DArray object and use the object's fields to access its contents. If it needs more information than it currently has, then change the _format_default_value function to take value instead of value.default_value.

Default Value for Double2DArray before reconstruct -
rows: 2
columns: 3
data: 1
data: 2
data: 3
data: 4
data: 5
data: 6

After Reconstruct -
'Double2DArray(rows=2, columns=3, data=[1, 2, 3, 4, 5, 6])'

Now, the error (without reconstruct) is thrown because the generated template has Double2DArray value as default_value=rows: 2\ncolumns: 3\ndata: 1\ndata: 2\ndata: 3\ndata: 4\ndata: 5\ndata: 6\n,\r\n

and it is passed to this method

AssertionError: assert 1 == 0 E + where 1 = <Result InvalidInput('Cannot parse: 242:34: default_value=rows: 2')>.exit_code tests\acceptance\test_client_generator.py:44: AssertionError

@bkeryan If I understand correctly, your point here is that default_value should contain the python data type (Double2DArray in this case) and any formatting should be done in the _format_default_value method defined in the template file.
_format_default_value should be checking if default_value is an instance of Double2DArray and do the necessary steps to convert it to the 'Double2DArray(rows=2, columns=3, data=[1, 2, 3, 4, 5, 6])' string.

That makes sense.

If the above understanding is correct then, one concern is that currently the default_value is added to the generated file not only through the _format_default_value method. Here the default value is being used directly and the reason that things work fine is because this piece of code incorporates the special string logic in _format_default_value.
Is that intentional?
If yes then, whatever Double2DArray conversion logic that is added into _format_default_value will need to be added here as well, or we would need this code to be passed through the _format_default_value workflow.

@clintverghese It may look like this is duplicated code, but get_configuration_parameters_with_type_and_default_values and _format_default_value are doing slightly different things.

get_configuration_parameters_with_type_and_default_values generates Python code for the client API class's measure() parameter list. In some cases we use more Pythonic data types in the client API. For example, path parameters are pathlib.Path in the client API and str in protobuf.

_format_default_value is used when generating Python code for the service's metadata structs. This tends to use the protobuf types or basic Python types, not stuff like pathlib.Path.

I think it would be fine to single-source _format_default_value by moving it into _support.py so that get_configuration_parameters_with_type_and_default_values can call it for non-path cases. Also, it would be helpful to update the docstrings to clarify this difference.

Moved the _format_default_value method to _support.py and added logic for Double2DArray. Now, we are calling this method from get_configuration_parameters_with_type_and_default_values for string and Double2dArray parameter types in _support.py and for the default value conversions in the template. Also, updated Docstring for both the methods.

dixonjoel · 2024-12-04T15:40:36Z

Please fill out the description especially what testing you've done interactively. I'll wait for review until the base branch goes into main. I don't see any major problems with the PR.

bkeryan · 2024-12-04T16:32:58Z

packages/generator/ni_measurement_plugin_sdk_generator/client/_support.py

@@ -209,6 +212,18 @@ def get_configuration_and_output_metadata_by_index(
            default_value = default_value.value
        elif isinstance(default_value, list) and any(isinstance(e, Enum) for e in default_value):
            default_value = [e.value for e in default_value]
+        elif isinstance(default_value, str):
+            default_value = repr(default_value)
+        elif configuration_metadata[id].message_type == "ni.protobuf.types.Double2DArray":


I disagree. I think this approach is unacceptable. str(some_double_2d_array) is for display purposes and not for parsing.

_format_default_value should downcast the default value to a Double2DArray object and use the object's fields to access its contents. If it needs more information than it currently has, then change the _format_default_value function to take value instead of value.default_value.

bkeryan · 2024-12-04T16:44:28Z

packages/generator/ni_measurement_plugin_sdk_generator/client/_support.py

+        elif isinstance(default_value, str):
+            default_value = repr(default_value)


The code used to work this way, but we changed it for a reason. Storing the Python representation of the default value in place of the default value is confusing and makes the code hard to audit for XSS-style issues: https://bandit.readthedocs.io/en/latest/plugins/b702_use_of_mako_templates.html

bkeryan · 2024-12-04T16:46:01Z

...rator/ni_measurement_plugin_sdk_generator/client/templates/measurement_plugin_client.py.mako

@@ -120,7 +120,7 @@ class ${class_name}:
                display_name=${value.display_name | repr},
                type=Field.Kind.ValueType(${value.type}),
                repeated=${value.repeated},
-                default_value=${_format_default_value(value.default_value)},
+                default_value=${value.default_value},


Removing this function and storing the Python representation of the default value in place of the default value is not acceptable. Plus you didn't even remove the function, you left it as dead code.

examples/sample_measurement/measurement.py

packages/generator/tests/utilities/measurements/non_streaming_data_measurement/__init__.py

examples/sample_measurement/measurement.py

…StreamingDataMeasurementClient

…gin client

…asurement plugin client

Blahpapto1 changed the title ~~Users/avgoel/add input support double2 d array~~ Input Support for Double2dArray Nov 29, 2024

Blahpapto1 force-pushed the users/avgoel/add_InputSupport_Double2DArray branch from 14c3349 to 2c936fc Compare December 4, 2024 07:22

Blahpapto1 changed the base branch from main to users/inkumarr/py-2dArray-support December 4, 2024 07:22

Blahpapto1 marked this pull request as ready for review December 4, 2024 07:30

Blahpapto1 requested review from csjall, dixonjoel, bkeryan, mshafer-NI and nstokes-nati as code owners December 4, 2024 07:30

dixonjoel reviewed Dec 4, 2024

View reviewed changes

bkeryan requested changes Dec 4, 2024

View reviewed changes

bkeryan reviewed Dec 4, 2024

View reviewed changes

examples/sample_measurement/measurement.py Show resolved Hide resolved

Blahpapto1 changed the title ~~Input Support for Double2dArray~~ Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement Dec 5, 2024

Base automatically changed from users/inkumarr/py-2dArray-support to main December 6, 2024 07:00

Blahpapto1 and others added 3 commits December 6, 2024 15:36

Add Double2DArray Input Configurations support

6c74d3e

Fix Lint issues

9dcf1dd

Refactor Double2DArray initialization for improved readability in Non…

c7fbf70

…StreamingDataMeasurementClient

Blahpapto1 force-pushed the users/avgoel/add_InputSupport_Double2DArray branch from b0e4d13 to c7fbf70 Compare December 6, 2024 10:41

Avi Goel added 3 commits December 6, 2024 17:21

Enhance default value formatting for Double2DArray in measurement plu…

eaa61c4

…gin client

Refactor docstrings for clarity and improve import organization in me…

4f1f838

…asurement plugin client

Refactor docstrings for consistency and clarity in _support.py

1f5551c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement #1026

Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement #1026

Blahpapto1 commented Nov 29, 2024 •

edited

Loading

Blahpapto1 commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

dixonjoel Dec 4, 2024

dixonjoel Dec 4, 2024

bkeryan Dec 4, 2024 •

edited

Loading

Blahpapto1 Dec 5, 2024

Blahpapto1 Dec 5, 2024

Blahpapto1 Dec 5, 2024

clintverghese Dec 5, 2024 •

edited

Loading

bkeryan Dec 5, 2024 •

edited

Loading

Blahpapto1 Dec 6, 2024

dixonjoel commented Dec 4, 2024

bkeryan Dec 4, 2024 •

edited

Loading

bkeryan Dec 4, 2024

bkeryan Dec 4, 2024

		elif isinstance(default_value, str):
		default_value = repr(default_value)

Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement #1026

Are you sure you want to change the base?

Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement #1026

Conversation

Blahpapto1 commented Nov 29, 2024 • edited Loading

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

Blahpapto1 commented Nov 29, 2024 • edited Loading

github-actions bot commented Nov 29, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkeryan Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintverghese Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

bkeryan Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dixonjoel commented Dec 4, 2024

bkeryan Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blahpapto1 commented Nov 29, 2024 •

edited

Loading

Blahpapto1 commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

bkeryan Dec 4, 2024 •

edited

Loading

clintverghese Dec 5, 2024 •

edited

Loading

bkeryan Dec 5, 2024 •

edited

Loading

bkeryan Dec 4, 2024 •

edited

Loading