Fixing size parameter handling in len-mechanism to handle function parameters with different size parameters by batzuu · Pull Request #2138 · ni/nimi-python

batzuu · 2025-11-07T13:54:29Z

This contribution adheres to CONTRIBUTING.md.
~~I've updated CHANGELOG.md if applicable.~~
I've added tests applicable for this pull request

What does this Pull Request accomplish?

Fixes the codegen to properly handle functions with multiple array parameters that reference different size parameters:

Updates metadata_filters.py: Refactors the filter_parameters function to collect all size parameter names referenced by len-sized parameters into a len_size_parameter_names, replacing the previous single len_size_parameter approach. Removed assert to check if all size parameters are same from filter_len_parameters. Following this updated var names in default_method.py.mako and initialization_method.py.mako to reflect multiple possible size params.
Updates to nifake to enable testing: Details covered in testing section

Previously, it was assumed all len-mechanism arrays in a function shared the same size parameter. This change enables functions like CreateDeembeddingSparameterTableArray in NI-RFSG where frequencies uses frequenciesSize and sparameterTable uses sparameterTableSize.

List issues fixed by this Pull Request

Issue 2137: Fix size parameter handling for functions with parameters referencing multiple size parameters.
Additional PR needs to be raised to update NI-RFSG function metadata (CreateDeembeddingSparameterTableArray) to consume
these changes.

What testing has been done?

Added MultipleArraysDifferentSize function to nifake metadata with two arrays (valuesArray using valuesArraySize, and dataArray using dataArraySize) to test the change in filter param logic
Added unit test test_multiple_arrays_different_size in test_library_interpreter.py that verifies correct handling of multiple arrays with independent size parameters
Test validates that each array's length is properly calculated and passed with its corresponding size parameter (3 elements for valuesArray with valuesArraySize, and 5 elements for dataArray with dataArraySize)
All existing tests continue to pass.

…handling

codecov-commenter · 2025-11-07T14:08:55Z

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (be77b68) to head (ce0a252).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
generated/nifake/nifake/_grpc_stub_interpreter.py	50.00%	1 Missing ⚠️
generated/nifake/nifake/session.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2138      +/-   ##
==========================================
+ Coverage   90.09%   90.12%   +0.03%     
==========================================
  Files          71       71              
  Lines       18662    18675      +13     
==========================================
+ Hits        16813    16831      +18     
+ Misses       1849     1844       -5

Flag	Coverage Δ
codegenunittests	`84.87% <100.00%> (+0.43%)`	⬆️
nidcpowersystemtests	`94.65% <ø> (+0.04%)`	⬆️
nidcpowerunittests	`89.53% <ø> (ø)`
nidigitalsystemtests	`92.26% <ø> (ø)`
nidigitalunittests	`68.44% <ø> (ø)`
nidmmsystemtests	`92.72% <ø> (ø)`
nifakeunittests	`85.60% <60.00%> (-0.11%)`	⬇️
nifgensystemtests	`94.61% <ø> (ø)`
nimodinstsystemtests	`73.85% <ø> (ø)`
nimodinstunittests	`94.20% <ø> (ø)`
nirfsgsystemtests	`82.40% <ø> (ø)`
niscopesystemtests	`92.94% <ø> (ø)`
niscopeunittests	`43.20% <ø> (ø)`
nisesystemtests	`91.50% <ø> (ø)`
niswitchsystemtests	`82.03% <ø> (ø)`
nitclksystemtests	`94.87% <ø> (ø)`
nitclkunittests	`98.26% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
build/helper/metadata_filters.py	`76.69% <100.00%> (+3.88%)`	⬆️
build/helper/metadata_find.py	`80.55% <100.00%> (+12.69%)`	⬆️
generated/nifake/nifake/_grpc_stub_interpreter.py	`82.80% <50.00%> (-0.27%)`	⬇️
generated/nifake/nifake/session.py	`77.62% <66.66%> (-0.07%)`	⬇️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be77b68...ce0a252. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ni-jfitzger · 2025-11-11T19:43:38Z

generated/nidcpower/nidcpower/_library_singleton.py:43:5: F824 global _instance_lock is unused: name is never assigned in scope
global _instance_lock

It seems we've gone from using flake8 7.1.2 to using flake8 7.3.0 and the new one has this new error code.
It doesn't like that we're using global here but never assigning a value to _instance_lock in that scope.
Though, according to the documentation, you need to declare with global before you can even use the variable.

https://docs.python.org/3/reference/simple_stmts.html#the-global-statement

This seems like something we should address in a separate PR.

bkeryan · 2025-11-11T20:22:43Z

Though, according to the documentation, you need to declare with global before you can even use the variable.

https://docs.python.org/3/reference/simple_stmts.html#the-global-statement

This seems like something we should address in a separate PR.

You only need the global keyword to assign global variables from an inner scope, not to read them.

https://stackoverflow.com/questions/291978/short-description-of-the-scoping-rules
https://docs.python.org/3/reference/executionmodel.html#resolution-of-names

batzuu · 2025-11-13T12:08:07Z

It seems we've gone from using flake8 7.1.2 to using flake8 7.3.0 and the new one has this new error code. It doesn't like that we're using global here but never assigning a value to _instance_lock in that scope. Though, according to the documentation, you need to declare with global before you can even use the variable.

https://docs.python.org/3/reference/simple_stmts.html#the-global-statement

This seems like something we should address in a separate PR.

I have added a noqa to suppress the flake8 warning so that this PR can merge. The actual issue we will address in a separate PR.

…andling

ni-jfitzger · 2026-02-13T15:08:57Z

build/helper/metadata_filters.py

    # If we are being called looking for the ivi-dance, len or code param, we do not care about the size param so we do
    #  not call back into ourselves, to avoid infinite recursion
    if parameter_usage_options not in [ParameterUsageOptions.IVI_DANCE_PARAMETER, ParameterUsageOptions.LEN_PARAMETER]:
-        # Find the size parameter - we are assuming there can only be one type, either from ivi-dance or len


# Find the size parameter - we are assuming there can only be one type, either from ivi-dance or len

You deleted this comment and the if statement guarding population of len size parameters.
But I don't see any testing for mixed usage.

Thanks Jay.
I added two unit tests that verify ivi-dance and len used together, including a case with multiple len size parameters to cover the mixed usage cases.

ni-jfitzger · 2026-02-13T15:36:49Z

build/helper/metadata_filters.py

+        len_params = filter_len_parameters(parameters)
+        for param in len_params:
+            len_size_param = find_size_parameter(param, parameters)
+            if len_size_param is not None:
+                len_size_parameter_names.add(len_size_param['name'])


I think it would be better to push most of this logic into a find_size_parameters or find_len_size_parameters or find_len_size_parameter_names function in metadata_find.py.

Yes.. I included find_len_size_parameter_names method in metadata_find.py which is used in this file.

ni-jfitzger · 2026-02-13T15:57:03Z

build/helper/metadata_filters.py

+        Empty list if no len parameter found
+        List of parameters if any are found


Suggested change

Empty list if no len parameter found

List of parameters if any are found

Empty list if no len parameter found

List of parameters dicts if any are found

ni-jfitzger · 2026-02-13T16:04:11Z

build/templates/_library_interpreter.py/default_method.py.mako

-    len_size_parameter = helper.find_size_parameter(len_parameters, parameters)
-    assert ivi_dance_size_parameter is None or len_size_parameter is None
+    len_size_parameters = helper.find_size_parameter(len_parameters, parameters)
+    assert ivi_dance_size_parameter is None or len_size_parameters is None


So we're not supporting mixed usage, after all?

We do support mixed usage.
That assert was removed, and mixed ivi-dance + len behavior is now covered by unit tests (including multiple len size parameters).

ni-jfitzger · 2026-02-13T19:53:59Z

build/templates/_library_interpreter.py/default_method.py.mako

    len_parameters = helper.filter_len_parameters(parameters)
-    len_size_parameter = helper.find_size_parameter(len_parameters, parameters)
-    assert ivi_dance_size_parameter is None or len_size_parameter is None
+    len_size_parameters = helper.find_size_parameter(len_parameters, parameters)


This new variable name is a misleading. helper.find_size_parameter only returns a single parameter.

Agreed. I have updated the name as before

ni-jfitzger · 2026-02-13T19:54:07Z

build/templates/_library_interpreter.py/initialization_method.py.mako

    len_parameters = helper.filter_len_parameters(parameters)
-    len_size_parameter = helper.find_size_parameter(len_parameters, parameters)
-    assert ivi_dance_size_parameter is None or len_size_parameter is None
+    len_size_parameters = helper.find_size_parameter(len_parameters, parameters)


This new variable name is a misleading. helper.find_size_parameter only returns a single parameter.

I have updated the same pattern in both mako files

ni-jfitzger · 2026-02-13T20:44:51Z

src/nifake/metadata/functions.py

+                'name': 'dataArraySize',
+                'type': 'ViInt32',
+                'use_array': False,
+                'use_in_python_api': False


_add_use_in_python_api should be able to set this to False for the size parameters of type 'len'. If you're having to set it yourself, I think that could be considered a bug.

Removed and build tox. No issues found. Thanks for highlighting it Jay

Utkarsh Tyagi added 3 commits November 7, 2025 15:34

changes to handle functions with multiple length size params

10bd251

added MultipleArraysDifferentSize function in nifake to validate len …

ddf7097

…handling

changes var names

2700665

batzuu requested review from marcoskirsch and ni-jfitzger as code owners November 7, 2025 13:54

adding noqa to suppress flake error for global _instance_lock

bd858fd

This was referenced Nov 14, 2025

NI-RFSG gRPC enum metadata cleanup #2139

Merged

Fix len parameter handling: Unit tests for gRPC and session #2140

Draft

vnktshr21 mentioned this pull request Nov 21, 2025

remove global declaration for _instance_lock #2144

Merged

3 tasks

rahur-NI added 2 commits February 10, 2026 19:12

Merge remote-tracking branch 'origin/master' into fix-len-parameter-h…

287ad78

…andling

Removing global _instance_lock from _library_singleton.py.mako file

d2ee442

rahur-NI requested a review from vnktshr21 as a code owner February 11, 2026 07:01

Merge branch 'master' into fix-len-parameter-handling

9488807

ni-jfitzger reviewed Feb 13, 2026

View reviewed changes

Updating based on code review comments

ce0a252

		Empty list if no len parameter found
		List of parameters if any are found

Conversation

batzuu commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this Pull Request accomplish?

List issues fixed by this Pull Request

What testing has been done?

Uh oh!

codecov-commenter commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ni-jfitzger commented Nov 11, 2025

Uh oh!

bkeryan commented Nov 11, 2025

Uh oh!

batzuu commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ni-jfitzger Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ni-jfitzger Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

batzuu commented Nov 7, 2025 •

edited

Loading

codecov-commenter commented Nov 7, 2025 •

edited

Loading

batzuu commented Nov 13, 2025 •

edited

Loading

ni-jfitzger Feb 13, 2026 •

edited

Loading

ni-jfitzger Feb 13, 2026 •

edited

Loading