chore: Update pylint to 2.17.4 #24700

EugeneTorap · 2023-07-14T17:49:00Z

This PR bumps:

pylint to 2.17.4 because pylint has astroid 2.6.6 which depends on wrapt<1.13 and >=1.11

SUMMARY

If we use python 3.11 and want to run superset CLI command we have the next error:

ImportError: cannot import name 'formatargspec' from 'inspect' (/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/inspect.py)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2023-07-14T18:21:12Z

Codecov Report

Merging #24700 (d56d557) into master (0631a80) will increase coverage by 0.00%.
The diff coverage is 72.60%.

❗ Current head d56d557 differs from pull request most recent head ba9ea89. Consider uploading reports for the commit ba9ea89 to get more accurate results

@@           Coverage Diff           @@
##           master   #24700   +/-   ##
=======================================
  Coverage   68.95%   68.95%           
=======================================
  Files        1902     1902           
  Lines       73939    73997   +58     
  Branches     8176     8195   +19     
=======================================
+ Hits        50984    51025   +41     
- Misses      20842    20853   +11     
- Partials     2113     2119    +6

Flag	Coverage Δ
hive	`54.13% <48.22%> (-0.01%)`	⬇️
mysql	`79.19% <62.41%> (-0.01%)`	⬇️
postgres	`79.29% <62.41%> (-0.01%)`	⬇️
presto	`54.03% <47.51%> (-0.01%)`	⬇️
python	`83.35% <65.95%> (-0.01%)`	⬇️
sqlite	`77.86% <60.99%> (-0.01%)`	⬇️
unit	`54.95% <47.51%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
superset-frontend/src/components/Select/utils.tsx	`82.25% <ø> (-1.62%)`	⬇️
superset/charts/commands/delete.py	`92.30% <ø> (ø)`
superset/charts/filters.py	`88.40% <ø> (ø)`
superset/cli/importexport.py	`80.00% <ø> (ø)`
superset/cli/main.py	`0.00% <0.00%> (ø)`
superset/cli/update.py	`0.00% <ø> (ø)`
superset/commands/exceptions.py	`93.84% <ø> (ø)`
superset/commands/importers/v1/examples.py	`0.00% <0.00%> (ø)`
superset/common/query_context_factory.py	`76.81% <ø> (ø)`
superset/common/query_context_processor.py	`85.08% <ø> (ø)`
... and 85 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2023-07-14T21:17:30Z

superset/charts/data/api.py

@@ -143,7 +143,7 @@ def get_data(self, pk: int) -> Response:
            query_context = self._create_query_context_from_form(json_body)
            command = ChartDataCommand(query_context)
            command.validate()
-        except DatasourceNotFound as error:


I know some people find Pylint to be somewhat cumbersome, but I love to see Pylint every evolving which results in fixes like this—which typically flake8 et al. miss.

john-bodley · 2023-07-14T21:19:21Z

superset/commands/exceptions.py

@@ -44,7 +44,7 @@ def __init__(
        super().__init__(
            _(
                self.message_format.format(
-                    object_type, '"%s" ' % object_id if object_id else ""
+                    object_type, f'"{object_id}" ' if object_id else ""


Just wondering whether the trailing space should be there. I realize it was previously, but I'm not sure if that was a typo.

superset/db_engine_specs/ocient.py

superset/db_engine_specs/presto.py

requirements/development.in

john-bodley · 2023-07-14T21:26:10Z

Thanks @EugeneTorap for the PR. Technically we're not really bumping these packages, but more accurately re-compiling the frozen requirements by upgrading Pylint et al. which are used for CI, i.e., setup.py hasn't changed. Maybe the PR title can be updated to more accurately reflect this.

john-bodley · 2023-07-15T07:53:33Z

@EugeneTorap you can run Pylint locally via:

python3 -m pip install -r requirements/integration.txt
tox -e pylint

john-bodley

Thanks again @EugeneTorap for the cleanup PR. I took another pass and left some comments.

john-bodley · 2023-07-17T21:51:06Z

.pylintrc

@@ -86,6 +86,7 @@ disable=
    missing-docstring,
    duplicate-code,
    unspecified-encoding,
+    cyclic-import,


Why are we blanket disabling this message? This message is typically useful and thus I would be somewhat hesitant to ignore it globally.

There're hundreds of cyclic-import errors in the project.
@john-bodley I believe that your SIP-92 Proposal for restructuring the Python code base should solve the problem!

@EugeneTorap as far as I'm aware the cyclic-import error existed in Pylint 2.9.6 and thus I'm somewhat perplexed as why this is significantly more problematic now. In general we shouldn't be relaxing most Pylint rules and thus I'm somewhat hesitant to approve something which in theory could lead to a regression.

When I pulled your branch, removed line #89 from the .pylintrc file, and ran,

python3.9 -m pip install -r requirements/integration.txt tox -e pylint

I didn't see any cyclic-import errors being reported.

Note I did see some errors with the .pylintrc file:

.pylintrc:1:0: E0015: Unrecognized option found: optimize-ast, files-output, argument-name-hint, method-name-hint, variable-name-hint, inlinevar-name-hint, const-name-hint, class-name-hint, class-attribute-name-hint, module-name-hint, attr-name-hint, function-name-hint, no-space-check (unrecognized-option)

it seems like some of these options are likely deprecated.

john-bodley · 2023-07-17T21:52:46Z

requirements/base.txt

@@ -305,7 +305,7 @@ werkzeug==2.3.3
    #   flask
    #   flask-jwt-extended
    #   flask-login
-wrapt==1.12.1
+wrapt==1.15.0


I'm a tad perplexed how/where this was bumped as it's a non-direct requirement and as far as I can tell the deprecated package (which it is a direct requirement of) wasn't bumped.

When I updated pylint then pylint updated astroid then astroid updated wrapt to 1.15.0

superset/config.py

superset/connectors/sqla/views.py

superset/dashboards/api.py

superset/db_engine_specs/presto.py

superset/models/sql_lab.py

superset/sql_validators/presto_db.py

superset/tasks/cache.py

rusackas · 2023-07-19T16:45:05Z

@EugeneTorap if you don't mind rebasing to resolve the conflict, I think this one's ready to merge!

EugeneTorap · 2023-07-19T16:58:09Z

@rusackas I want to improve my PR tomorrow

# Conflicts: # superset/charts/commands/warm_up_cache.py # superset/views/core.py # superset/viz.py

# Conflicts: # requirements/development.txt

john-bodley · 2023-07-24T20:33:17Z

@EugeneTorap I pushed a commit which further bumped the version of Pylint (to the maximum allowable given other constraints) to see if this would address said issue. Regrettably it didn't and I'm somewhat at a loss in terms of i) why this is occurring, and ii) why I can't reproduce it locally.

The second commit I pushed was—like your prior commit—was to disable this globally given (as you pointed out previously) disabling on a per-file basis didn't seem to work.

Would you mind updating the PR title/description to depict that main impetus of this PR, i.e., bumping Pylint? I'll merge the PR after that.

Co-authored-by: John Bodley <[email protected]>

Bump wrapt lib

45c9ad4

pull-request-size bot added the size/XS label Jul 14, 2023

EugeneTorap added 2 commits July 14, 2023 20:53

Bump astroid lib

c75c72b

Bump pylint lib

fb833ef

pull-request-size bot added size/S and removed size/XS labels Jul 14, 2023

Bump pylint lib

2956ec8

EugeneTorap requested a review from a team as a code owner July 14, 2023 19:15

pull-request-size bot added size/M size/L and removed size/S size/M labels Jul 14, 2023

john-bodley approved these changes Jul 14, 2023

View reviewed changes

john-bodley reviewed Jul 14, 2023

View reviewed changes

requirements/development.in Outdated Show resolved Hide resolved

EugeneTorap force-pushed the chore/bump-wrapt-lib branch 3 times, most recently from 88d933f to 0b586f7 Compare July 15, 2023 06:45

Fix pylint

a0eb3b2

EugeneTorap force-pushed the chore/bump-wrapt-lib branch from 686768b to a0eb3b2 Compare July 15, 2023 07:45

EugeneTorap added 6 commits July 15, 2023 11:08

Bump pylint to 2.14.5

a9d6d5d

Fix pylint

3fc936b

Fix pylint

969bf33

Fix pylint

2c6e8c2

Fix pylint

f049a71

Merge branch 'master' into chore/bump-wrapt-lib

e522fc4

john-bodley reviewed Jul 17, 2023

View reviewed changes

EugeneTorap added 6 commits July 20, 2023 21:14

Merge branch 'master' into chore/bump-wrapt-lib

141f0b6

# Conflicts: # superset/charts/commands/warm_up_cache.py # superset/views/core.py # superset/viz.py

Use pylint 'disable-next' instead of 'disable' in config.py

6f20797

Use pylint: disable=consider-using-f-string

6e4120a

Use pylint: disable=consider-using-f-string

0af0841

Fix .pylintrc

6b4d916

Disable cyclic-import for temporary_cache

f5972f0

EugeneTorap force-pushed the chore/bump-wrapt-lib branch from ea95989 to f5972f0 Compare July 22, 2023 14:18

EugeneTorap and others added 2 commits July 22, 2023 17:21

Merge branch 'master' into chore/bump-wrapt-lib

132feb3

# Conflicts: # requirements/development.txt

bump Pylint

67a2b4a

pull-request-size bot added size/XL and removed size/L labels Jul 24, 2023

Temporarily disable cyclic-import

ba9ea89

EugeneTorap changed the title ~~fix: Update wrapt to 1.15.0 to support Python 3.11~~ chore: Update pylint to 2.17.4 Jul 24, 2023

john-bodley merged commit fc89718 into apache:master Jul 25, 2023

EugeneTorap deleted the chore/bump-wrapt-lib branch July 25, 2023 00:22

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore: Update pylint to 2.17.4 (apache#24700)

67fbb2d

Co-authored-by: John Bodley <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Update pylint to 2.17.4 #24700

chore: Update pylint to 2.17.4 #24700

EugeneTorap commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

john-bodley Jul 14, 2023

john-bodley Jul 14, 2023

john-bodley commented Jul 14, 2023

john-bodley commented Jul 15, 2023

john-bodley left a comment

john-bodley Jul 17, 2023

EugeneTorap Jul 20, 2023 •

edited

Loading

john-bodley Jul 20, 2023

john-bodley Jul 17, 2023

EugeneTorap Jul 20, 2023

rusackas commented Jul 19, 2023

EugeneTorap commented Jul 19, 2023

john-bodley commented Jul 24, 2023 •

edited

Loading

chore: Update pylint to 2.17.4 #24700

chore: Update pylint to 2.17.4 #24700

Conversation

EugeneTorap commented Jul 14, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 14, 2023 • edited Loading

Codecov Report

john-bodley Jul 14, 2023

Choose a reason for hiding this comment

john-bodley Jul 14, 2023

Choose a reason for hiding this comment

john-bodley commented Jul 14, 2023

john-bodley commented Jul 15, 2023

john-bodley left a comment

Choose a reason for hiding this comment

john-bodley Jul 17, 2023

Choose a reason for hiding this comment

EugeneTorap Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

john-bodley Jul 20, 2023

Choose a reason for hiding this comment

john-bodley Jul 17, 2023

Choose a reason for hiding this comment

EugeneTorap Jul 20, 2023

Choose a reason for hiding this comment

rusackas commented Jul 19, 2023

EugeneTorap commented Jul 19, 2023

john-bodley commented Jul 24, 2023 • edited Loading

EugeneTorap commented Jul 14, 2023 •

edited

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

EugeneTorap Jul 20, 2023 •

edited

Loading

john-bodley commented Jul 24, 2023 •

edited

Loading