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

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

Closed
XueSongTap opened this issue Oct 17, 2024 · 3 comments · Fixed by #158
Closed

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

XueSongTap opened this issue Oct 17, 2024 · 3 comments · Fixed by #158
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@XueSongTap
Copy link
Contributor

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?

@XueSongTap XueSongTap added the kind/bug Categorizes issue or PR as related to a bug. label Oct 17, 2024
@Yoda-wu
Copy link
Contributor

Yoda-wu commented Oct 17, 2024

dataset/split_dataset.py related pr: #143

@FuryMartin
Copy link
Contributor

FuryMartin commented Oct 18, 2024

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

  • Pick Solution 2 temporarily in order to merge OSPP projects.
  • Solution 3 will be considered as a long-term option for future adoption.

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
Copy link
Collaborator

MooreZheng commented Oct 18, 2024

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants