You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2024/04/04 00:24:00 UTC
[PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
john-bodley opened a new pull request, #27889:
URL: https://github.com/apache/superset/pull/27889
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
This PR fixes CI in `master` which is broken due to https://github.com/apache/superset/pull/27867.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
CI.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #27889:
URL: https://github.com/apache/superset/pull/27889#issuecomment-2035859229
@mistercrunch this seems to be a byproduct of you merging https://github.com/apache/superset/pull/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 ad additional check which requires that the PR be rebased immediately prior to merging to help reduce the potential of these types of regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27889:
URL: https://github.com/apache/superset/pull/27889#discussion_r1550679498
##########
superset/db_engine_specs/presto.py:
##########
@@ -1329,7 +1329,9 @@ def handle_cursor(cls, cursor: Cursor, query: Query) -> None:
completed_splits,
total_splits,
)
- if progress > query.progress:
+ if ( # pylint: disable=consider-using-min-builtin
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #27889:
URL: https://github.com/apache/superset/pull/27889
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27889:
URL: https://github.com/apache/superset/pull/27889#issuecomment-2037603813
This should help prevent the issues experienced in this PR -> https://github.com/apache/superset/pull/27904
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27889:
URL: https://github.com/apache/superset/pull/27889#issuecomment-2035956932
I'm going merge now since this fixes `master`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27889:
URL: https://github.com/apache/superset/pull/27889#issuecomment-2037602842
I think the issue https://github.com/apache/superset/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #27889:
URL: https://github.com/apache/superset/pull/27889#issuecomment-2035892857
@mistercrunch looking into the checks for https://github.com/apache/superset/pull/27867 it seems like the logic is wrong in terms of eligibility, i.e., the following jobs didn't run:
- [pre-commit (3.9)](https://github.com/apache/superset/actions/runs/8533736544/job/23376904109)
- [python-lint (3.9)](https://github.com/apache/superset/actions/runs/8533736544/job/23376904480)
- [python-lint (3.10)](https://github.com/apache/superset/actions/runs/8533736544/job/23376904970)
as well as all the Python unit and integration tests, because it perceived it to be a no-op step.
Although these changes don't touch any Python files they have the potential to impact the entire Python code base and thus 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(pylint): Address errors/warnings introduced by #27867 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27889:
URL: https://github.com/apache/superset/pull/27889#issuecomment-2035946232
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org