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] Update dummy-variable regex for pylint #17206

Merged

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, the regex used for pylint to identify dummy variables would correctly identify variables that start with an underscore (e.g. _scale), unless they have an underscore elsewhere in the name (e.g. _scale_factor). This leads to false positives from pylint for unused variables, as prefixing a variable with an underscore should mark a variable as intentionally unused.

This commit updates the regex in TVM's pylintrc to match the current default value for dummy-variables-rgx, to allow unused variables to be named with a leading underscore, even if they also contain another underscore.

Prior to this commit, the regex used for pylint to identify dummy
variables would correctly identify variables that start with an
underscore (e.g. `_scale`), unless they have an underscore elsewhere
in the name (e.g. `_scale_factor`).  This leads to false positives
from pylint for unused variables, as prefixing a variable with an
underscore should mark a variable as intentionally unused.

This commit updates the regex in TVM's `pylintrc` to match the current
default value for `dummy-variables-rgx`, to allow unused variables to
be named with a leading underscore, even if they also contain another
underscore.
@Lunderberg
Copy link
Contributor Author

I ran into this false positive for #17201 for a variable named _in_group (screenshot below). While I could work around it by renaming to _ingroup, it's also good to have a long-term fix for the lint rule.

image

Copy link
Contributor

@quic-sanirudh quic-sanirudh left a comment

Choose a reason for hiding this comment

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

Since we're already using names starting with _ to represent dummy variables, it makes sense to allow _ in other places as part of the name.

One thing though is that PEP 8 style guide suggests using names starting with _ to be used as "weak" internal use variables, and the convention I've used till now is that unused variables are named with just an _ like x, _ = get_xy(). So, my question is why do we want to name unused variables in general? Could we not just use _?

@Lunderberg
Copy link
Contributor Author

So, my question is why do we want to name unused variables in general? Could we not just use _?

Mainly to indicate to a reader which return values are being ignored. As an example, suppose there's a function that produces a

# A reader may not know what the first two return values are.
_, _, attrs = define_function(relax_args)

# The first two return values are a function and parameters to 
# that function.
_func, _params, attrs = define_function(relax_args)

I've run into a few cases where the same return value was re-generated, because a later developer wasn't aware that it was already being generated. By explicitly naming the unused return value, this becomes less likely. (e.g. Seeing _func instead of _, and checking whether it is already the function that should be used.)

Obviously, this shouldn't be applied in all cases, as _, y = get_xy() is already perfectly readable, but it can help in cases where it isn't as clear.

@quic-sanirudh
Copy link
Contributor

So, my question is why do we want to name unused variables in general? Could we not just use _?

Mainly to indicate to a reader which return values are being ignored. As an example, suppose there's a function that produces a

# A reader may not know what the first two return values are.
_, _, attrs = define_function(relax_args)

# The first two return values are a function and parameters to 
# that function.
_func, _params, attrs = define_function(relax_args)

I've run into a few cases where the same return value was re-generated, because a later developer wasn't aware that it was already being generated. By explicitly naming the unused return value, this becomes less likely. (e.g. Seeing _func instead of _, and checking whether it is already the function that should be used.)

Obviously, this shouldn't be applied in all cases, as _, y = get_xy() is already perfectly readable, but it can help in cases where it isn't as clear.

Makes sense, thanks for your reply.

@quic-sanirudh quic-sanirudh merged commit 9e88018 into apache:main Jul 30, 2024
19 checks passed
@Lunderberg Lunderberg deleted the ci_pylint_dummy_variable_regex branch July 30, 2024 15:46
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.

2 participants