fix(pylint): Address errors/warnings introduced by #27867

[john-bodley]

SUMMARY

This PR fixes CI in master which is broken due to #27867.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

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

[john-bodley]

@mistercrunch this seems to be a byproduct of you merging #27867.

I realize that there has been a number of recent changes on master with regards to GitHub actions and workflows, Python dependencies (be that the introduction of pyproject.toml, the reduction of the number of environments, or the introduction of the supersetbot), etc. but the master branch has become somewhat unstable during this period which has negatively impacted the already fragile developer experience.

For changes emanating from supersetbot et al. I wonder if there could be an additional check which requires that the PR be rebased immediately prior to merging to help prevent regressions on master.

[john-bodley]

I opted not to use the suggestion, i.e., query.progress = max(progress, query.progress) as this would likely dirty the SQLAlchemy object and result in an excess of commits.

[john-bodley]

@mistercrunch looking into the checks for #27867 it seems like the logic is wrong in terms of eligibility, i.e., the following jobs didn't run:

• python-lint (3.9)
• python-lint (3.10)

because it wrongly perceived it to be a no-op step. Furthermore none of the checks in #27867 are listed as being Required which doesn't seem right.

Although these changes don't touch any Python files, bumping any Python package has the potential to impact the integrity of the codebase which Pylint may detect. It seems imperative that all Python checks are enabled before anymore supersetbot PRs related to bumping Python packages are merged.

Additionally all currently opened supersetbot PRs which attempt to bump a Python package should be rebased once this issue has been remedied in order for the full CI suite to run.

[mistercrunch]

Agreed. I've been creating havoc getting deep into CI. I'm really hoping we'll be much better on the other side and that things settle soon.

This particular issue here was around the fact I overlooked that CI only triggers if superset/** is touched.

Some of the cleanup was long overdue, but really hope I could make it less disruptive on the repo. I'll raise my level of caution, and slow the rhythm a bit.

[mistercrunch]

I'm going merge now since this fixes master

[mistercrunch]

I think the issue #27867 is that it didn't touch any files in superset/** (as specified here https://github.com/apache/superset/blob/master/.github/workflows/superset-python-misc.yml#L14) which means the "no-op" check took precedence over the skipped check, as expected.

Now adding requirements/** to the line bellow the one I pointed to above might help, saying "if we change a python dep, run the python checks in this file". (I'll write a PR for this)

About GHA checks, there are these core issues I'd like to address:

• there's no "this check is required to succeed, but only if it's triggered" - the no-ops are a work around this issue
• there's no "the whole matrix of checks need to succeed" semantics, meaning if you have a 4*3 matrix you need to spell out the 12 required checks...
• there's no good mechanism to keep things DRY in actions (simply referencing a global.main-python-version) in multiple files isn't possible. There's the concept of reusable actions and workflows, but basic templating would go a long way.
• the required checks source of truth are the ones specified in the main branch, which means deprecating a check can only be done through some sort of no-op, that then needs to get cleaned up once merge
• secrets are sometimes available, sometimes not

One more issue that has more to do on how we use GHA than its mechanics, is we have a bunch of single-item matrix (python-version: ["3.10"]), which created challenges around the issue above when migrating. For this I'm thinking de-matrixing things to simplify things a little, so the next python upgrade wouldn't mean creating a bunch of new no-ops.

[mistercrunch]

This should help prevent the issues experienced in this PR -> #27904