kubeedge / ianvs

Distributed Synergy AI Benchmarking
https://ianvs.readthedocs.io
Apache License 2.0
115 stars 46 forks source link

Pylint Errors Due to Compatibility Issues with Pylint3.9 #157

Closed XueSongTap closed 1 month ago

XueSongTap commented 1 month ago

Description:

Currently, this repository runs CI checks using pylint versions 3.7, 3.8, and 3.9.

In pylint 3.9, a new check too-many-positional-arguments was introduced. This check is triggering warnings in core/testenvmanager/dataset/dataset.py at several locations:

core/testenvmanager/dataset/dataset.py:119:4: R0917: Too many positional arguments (8/5) (too-many-positional-arguments)
core/testenvmanager/dataset/dataset.py:206:4: R0917: Too many positional arguments (6/5) (too-many-positional-arguments)
core/testenvmanager/dataset/dataset.py:213:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments)
core/testenvmanager/dataset/dataset.py:246:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments)
core/testenvmanager/dataset/dataset.py:285:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments)
core/testenvmanager/dataset/dataset.py:329:4: R0917: Too many positional arguments (7/5) (too-many-positional-arguments)
core/testenvmanager/dataset/dataset.py:368:4: R0917: Too many positional arguments (6/5) (too-many-positional-arguments)

Long-term solution:

Refactor the affected function definitions to reduce the number of positional arguments.

Short-term solution:

Add the following directive to disable the too-many-positional-arguments check:

# pylint: disable=too-many-positional-arguments

Problem:

Adding this directive causes pylint versions 3.7 and 3.8 to raise an error because they do not recognize the too-many-positional-arguments message:

core/testenvmanager/dataset/dataset.py:119:0: W0012: Unknown option value for 'disable', expected a valid pylint message and got 'too-many-positional-arguments' (unknown-option-value)

Question:

I'm not sure how to resolve this issue in the short term. One possible solution is to keep only pylint 3.9 in the CI pipeline, as Python 3.9 is backward compatible with 3.7, and pylint 3.9 provides stricter checks. Would this be an acceptable approach, or is there a better way to handle this situation?

Yoda-wu commented 1 month ago

dataset/split_dataset.py related pr: https://github.com/kubeedge/ianvs/pull/143

FuryMartin commented 1 month ago

Cause analysis

Pylint introduced a new code style error R0917:too-many-positional-arguments in 2023, which will throw an error when the number of function parameters exceeds the default value 5. See details at PylintDocs/R0917

This feature only exists in Pylint with python>=3.9, so Pylint with python=3.7,3.8 will not raise this error.

Possible Solutions

Solution 1: Disable R0917 Check

✅ Can Solve ⚠️ It will not be monitored when there are truly too many positional arguments.

Solution 2: Increase max-positional-arguments to A Bigger Number

✅ Can Solve ❓The upper limit still needs to be discussed. ( temporarily set to 10 in #158 )

Solution 3: Refactor dataset.py to Comply With the New Style Standards.

✅ Can Solve ⚠️ It may lead to large-scale interface changes, resulting in compatibility issues for historical projects.

Final Decision

Implementation Details

Since max-positional-arguments does not exist in pylint for python=3.7, 3.8, we cannot configure it uniformly through the .pylintrc file; otherwise, a new error will occur:

pylint: error: Unrecognized option found: max-positionational-arguments=10

We must attach different run commands for different versions of pylint by adding an 'if-else' branch to main.yaml to configure it for python=3.9

      run: |
          if [ "${{ matrix.python-version }}" = "3.9" ]; then
            pylint --max-positional-arguments=10 '${{github.workspace}}/core'
          else
            pylint '${{github.workspace}}/core'
          fi
MooreZheng commented 1 month ago

As for solution 3 of split_dataset.py, the function parameters could be looked over further. For example, parameters of "ratio" and "type" seem can be refined, considering the existing parameter of "method".