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

Fix bug with unclear function signature #8668

Merged

Conversation

hmc-cs-mdrissi
Copy link
Contributor

@hmc-cs-mdrissi hmc-cs-mdrissi commented May 9, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Fixes long open issue of getting tensorflow function signatures incorrect and producing false positives. Due to import/function definition of tensorflow functions sometimes infer finds multiple function definitions and picks wrong one. safe_infer does check that each function definition has same number of arguments, but it's possible the names/defaults vary.

Also added a few simple test cases. I manually tested with tf.concat and confirmed it fixed that false positive.

Closes #3613

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #8668 (8a5e049) into main (81917bc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8668   +/-   ##
=======================================
  Coverage   95.84%   95.85%           
=======================================
  Files         173      173           
  Lines       18492    18510   +18     
=======================================
+ Hits        17724    17742   +18     
  Misses        768      768           
Impacted Files Coverage Ξ”
pylint/checkers/utils.py 95.76% <100.00%> (+0.07%) ⬆️

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code Lib specific πŸ’… This affect the code from a particular library labels May 9, 2023
@github-actions

This comment has been minimized.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks great ! I think formatting the args make sense (it seems like it could be a performance hit a lot is done in this function). It does simplify the code and fix the issue though... Could you add a fragment for the changelog, please ?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0a7 milestone May 16, 2023
@jacobtylerwalls jacobtylerwalls changed the title Fix bug with unclear function signature #3613 Fix bug with unclear function signature Jun 18, 2023
@jacobtylerwalls
Copy link
Member

I think formatting the args make sense (it seems like it could be a performance hit a lot is done in this function).

Agree. Let's try just comparing argnames() instead.

@github-actions

This comment has been minimized.

@jacobtylerwalls
Copy link
Member

We're touching safe_infer() so let's not backport this.

@github-actions
Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on pandas:
The following messages are no longer emitted:

  1. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/0bc16da1e53e9a4d637cf81c770e6517da65a029/pandas/tests/arithmetic/test_datetime64.py#L699
  2. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/0bc16da1e53e9a4d637cf81c770e6517da65a029/pandas/tests/frame/test_arithmetic.py#L1096
  3. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/0bc16da1e53e9a4d637cf81c770e6517da65a029/pandas/tests/frame/test_arithmetic.py#L1097

This comment was generated for commit 8a5e049

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The primer looks great !

@jacobtylerwalls jacobtylerwalls merged commit 49653f4 into pylint-dev:main Jun 19, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Lib specific πŸ’… This affect the code from a particular library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pylint gets TensorFlow's tf.split all wrong
3 participants