Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Apply linting rules to AOT tests #11657

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Conversation

Mousius
Copy link
Member

@Mousius Mousius commented Jun 9, 2022

This enables pylint against the AOT test cases.

One issue I found was with the tvm.testing.parameter which breaks the naming convention rules in pylint (constants are upper case and function parameters are lower case). It may be worth a syntax similar to:

tvm.testing.parameter("enable_usmp", [True, False])

cc @areusch @driazati

@areusch
Copy link
Contributor

areusch commented Jun 10, 2022

cc @Lunderberg about the issue with linting tvm.testing.parameter.

Comment on lines 182 to 183
+ "with packed calling convention which is not supported by the NPU codegen's "
+ "TIR to Runtime Hook. We need to use a different target to test this feature"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
+ "with packed calling convention which is not supported by the NPU codegen's "
+ "TIR to Runtime Hook. We need to use a different target to test this feature"
"with packed calling convention which is not supported by the NPU codegen's "
"TIR to Runtime Hook. We need to use a different target to test this feature"

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mousius approved modulo one comment

@@ -151,7 +158,7 @@ def test_device_api_hooks_unpacked_api(device_api_main_func):
# We dont need to check exact input and output var names in this test.
# Hence, using a regex to cover any legal I/O name.
regex = re.compile(
'tir\.tvm_check_return\(0, -1, tir\.call_extern\("tvmgen_default_ethos_u_main_0", \w+, \w+, device_context_ethos_u\)\)'
r'tir\.tvm_check_return\(0, -1, tir\.call_extern\("tvmgen_default_ethos_u_main_0", \w+, \w+, device_context_ethos_u\)\)' # pylint: disable=line-too-long
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you break this up rather than disabling? e.g.

r'tir\.tvm_check_return\('
r'0, -1, '
r'tir\.call_extern\("tvmgen_default_ethos_u_main_0", '
r'\w+, \w+, device_context_ethos_u\)\)'

@Lunderberg
Copy link
Contributor

cc @Lunderberg about the issue with linting tvm.testing.parameter.

This was something that I had experimented with when writing it, and there isn't a good way around it. The exact name must match in both the global scope of the module and in the argument list of the test function, so there is no name that would satisfy both UPPER_CASE_CONSTANT and lower_case_parameter. The overall constraints were:

  • The name of the pytest fixture must be an exact match to its later use.
  • The pytest fixture must appear in global scope of the module. There were some cases which IIRC were related to caching, where using inspect to place it in the calling scope wouldn't be sufficient, because the calling scope may not be the parent scope. Walking up until we reach module scope wouldn't work, as parameters/fixtures may be at class scope instead.
  • The pytest fixture must appear in the argument names of the test that uses it. There are workarounds, such as request.getfixturevalue, but those break test parametrization, because the fixture use isn't known until run-time, but is required at collect-time.

@areusch
Copy link
Contributor

areusch commented Jun 13, 2022

@Lunderberg could we do something like, in e.g. testing.py:

@pytest.fixture(name='parameter')
def Parameter(*args):
  ...

parameter = Parameter

see https://docs.pytest.org/en/7.1.x/reference/reference.html#pytest-fixture

This enables pylint against the AOT test cases.

One issue I found was with the `tvm.testing.parameter` which breaks the naming convention rules in pylint (constants are upper case and function parameters are lower case). It may be worth a syntax similar to:

```
tvm.testing.parameter("enable_usmp", [True, False])
```
@Mousius Mousius merged commit 24f49f1 into apache:main Jun 15, 2022
@Mousius Mousius deleted the lint-aot-tests branch June 15, 2022 14:12
@Lunderberg
Copy link
Contributor

@areusch We can, and that's basically what the tvm.testing.parameter function does internally (link).

The main issue comes when pytest is finding the fixtures to be used, and avoiding the need to repeatedly define the fixture name. I had tracked it down to the the function that locates fixtures to convince myself that it needed to be a named object in the file. (I been hoping that there would be some pytest variable that could hold the fixture objects, rather than requiring a python variable.)

  • No parameter-specific support. Test writers would call pytest.fixture directly to define the parameter. This would pass the linting for lower_case function names, but gets to be rather clunky and copy-paste-y when defining several parameters.

    @pytest.fixture(params=[1, 2, 3])
    def my_parameter(request):
        return request.param
  • Repeat the parameter name. This would allow the ALL_CAPS name for the python variable, while having the fixture be named in lower_case. I felt this would be confusing, since there would exist two names and it would be unclear which one should be used in the unit test.

    MY_PARAMETER = tvm.testing.parameter(1, 2, 3, name='my_parameter')
  • Insert a dummy variable in the callee's scope to define a variable at the scope. This would work and wouldn't introduce any lint issues, but would break if tvm.testing.parameter is ever called as a subroutine, since the callee's scope wouldn't be the appropriate place for the inserted variable.

    tvm.testing.parameter(1, 2, 3, name='my_parameter')
  • Use the python variable name as the fixture name. This is the behavior I settled on, because it minimizes boilerplace from the user, doesn't require separate variable/fixture names, and wouldn't use fragile inspect.stack hackery.

    my_parameter = tvm.testing.parameter(1, 2, 3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants