-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Tracking Issue] pylint the tests! #11414
Comments
@gigiblender is going to look at this one |
@gigiblender can you put together a list of tests which need linting so we can put a checklist on this issue? then people can pick them off when they get time 😸 |
Checklist attached: |
Thanks @gigiblender, I've put them up here, if we can reference this issue in PRs then we'll get through this! |
This fixes up the pylint issues as part of apache#11414 for the CI tests
This fixes up the pylint issues as part of apache#11414 for the CI tests
This fixes up the pylint issues as part of apache#11414 for the CI tests
This fixes up the pylint issues as part of apache#11414 for the CI tests
I'll start with these:
|
This fixes up the pylint issues as part of apache#11414 for the CI tests
This fixes up the pylint issues as part of apache#11414 for the CI tests
I want to try these.
|
Hey folks, a lot of the files I see have one character variable names, mostly scheduling code. I think this is reasonable in many situations so think we should upgrade pylint, and use new pylint features to allow for more variable names. See #11712. Please let me know what you think. |
@AndrewZhaoLuo agreed, additionally, pylint seems to be inconsistent as far as which one (or two) character variable names it allows and which it doesn't. |
I also hit an issue in tests/python/unittest/test_micro_project_api.py related to pytest fixtures and the redefining name from outer scope warning, which is further complicated by tvm.testing.fixture() not supporting the name argument. For now I just disabled the warning. |
Update my issue from #11703 to #12028
|
Has anyone found a way to address the redefined-outer-name warning when tvm.testing.parameters is used? For example see test_crt.py::test_aot_executor_usmp_const_pool
Also, there's a related issue with the actual variable names as pylint wants the globals to be constants:
This is probably the same situation as to what we discussed previously above, but just checking before I disable these two warnings. |
@alanmacd I don't think there's any good solution found yet 😞 I went with replacing them in the tests I fixed up (actually the same |
Thanks, that works! It was slightly more complicated, but I think I figured it out. 😄 |
Would it make sense to create multiple PRs with a few files fixed in each PR? I'm working on the hexagon tests and a number of them are being regularly updated, so I think it might be easier to do them in parts. |
@quic-sanirudh yeah that's totally fine. you'll just need to slowly add those per-pr to the lint script. |
Thanks for the reply @areusch. I've raised the patch for the first set of files in #12082 |
Hi, I got a naive question about the issue and hope you to tell me whether my understanding is right or not. |
ccing @Lunderberg for visibility on the pylint/parametrize issue |
Second set of **hexagon tests** modified to be pylint compliant as part of #11414 tracking issue. The files supported in this patch are: * [X] tests/python/contrib/test_hexagon/test_autotvm.py * [X] tests/python/contrib/test_hexagon/test_cache_read_write.py * [X] tests/python/contrib/test_hexagon/test_launcher.py * [X] tests/python/contrib/test_hexagon/test_maxpool2d_blocked.py * [X] tests/python/contrib/test_hexagon/test_models.py * [X] tests/python/contrib/test_hexagon/test_run_unit_tests.py * [X] tests/python/contrib/test_hexagon/test_thread_pool.py * [X] tests/python/contrib/test_hexagon/test_usmp.py
This pr fixes pylint errors in tests/python/contrib/test_ethosn as reported in issue #11414.
) Second set of **hexagon tests** modified to be pylint compliant as part of apache#11414 tracking issue. The files supported in this patch are: * [X] tests/python/contrib/test_hexagon/test_autotvm.py * [X] tests/python/contrib/test_hexagon/test_cache_read_write.py * [X] tests/python/contrib/test_hexagon/test_launcher.py * [X] tests/python/contrib/test_hexagon/test_maxpool2d_blocked.py * [X] tests/python/contrib/test_hexagon/test_models.py * [X] tests/python/contrib/test_hexagon/test_run_unit_tests.py * [X] tests/python/contrib/test_hexagon/test_thread_pool.py * [X] tests/python/contrib/test_hexagon/test_usmp.py
This pr fixes pylint errors in tests/python/contrib/test_ethosn as reported in issue apache#11414.
This is a tracking issue for our effort to enable pylint on all Python testing code underneath
tests/python
. Currently we don't lint scripts in that directory. We're going to take an incremental approach here and anyone is welcomed to help with this. Here's how:tests/lint/pylint.sh
containing that directory similar to Making CMSIS-NN tests pylint compliant #11625.docker/lint.sh pylint
locally and fix the lint issues.Here are the tests that need linting 😸 (we will try to tag the PR which fixes them when we merge them):
The text was updated successfully, but these errors were encountered: