You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mistercrunch (via GitHub)" <gi...@apache.org> on 2024/04/04 17:34:13 UTC
[PR] chore: [proposal] de-matrix python-version in GHAs [superset]
mistercrunch opened a new pull request, #27906:
URL: https://github.com/apache/superset/pull/27906
### SUMMARY
In this PR:
- simplifying the single-item matrix (python-version) to NOT using a
matrix. I'm guessing the reason we currently have a single-item matrix
is an artifact of supporting multiple version in the past, and/or making it
easy to go multi-python-version checks in the future, but there's a burden
associated, especially around how this relates to "required checks"
specified in .asf.yml
- leveraging the `setup-backend`'s default for python version, making
the main python version we use much more DRY.
- fixing/simplifying the related no-op workflows. We'll need new ones, but will
be able to deprecate a bunch and simplify things. For instance,
when we migrate to 3.11 in the future, we won't have to manage
a bunch of python-version-specific no-ops
About supporting multiple/future version of python, I'd argue that we
should focus on a single one for a given CI run, and that if/when we
need to CI against multiple version, we run a FULL test suite
punctually in a dedicate PR/branch/ref. Point being, it's expensive
for every commit to validate multiple versions of python and in
many ways its not necessary.
Currently our multi-python-version support is dubious at best, with only
few checks that run against multiple versions. I really think
we should pick a single version and support it very well. If/when we
want to upgrade python version, we'd cut a PR and run CI for that
purpose.
If we want to continuously, actively support multiple python versions
(and I don't think we should!), I'd suggest either
a release-specific procedure (release manager using
release branch, running full CI for that version/release) and/or a
nightly job that would keep an eye on that version of python.
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #27906:
URL: https://github.com/apache/superset/pull/27906#discussion_r1560036976
##########
.github/workflows/no-op.yml:
##########
@@ -0,0 +1,96 @@
+# no-op.yml
+#
+# Purpose:
+# This workflow provides a workaround for the "required status checks" feature in GitHub Actions
+# when using path-specific conditions in other workflows. Required checks might remain in a "Pending"
+# state if the conditions are not met, thus blocking pull requests from being merged.
+# This no-op (no operation) workflow provides dummy success statuses for these required jobs when
+# the real jobs do not run due to path-specific conditions.
+#
+# How it works:
+# - It defines jobs with the same names as the required jobs in the main workflows.
+# - These jobs simply execute a command (`exit 0`) to succeed immediately.
+# - When a pull request is created or updated, both this no-op workflow and the main workflows are triggered.
+# - If the main workflows' jobs don't run (due to path conditions), these no-op jobs provide successful statuses.
+# - If the main workflows' jobs do run and fail, their failure statuses take precedence,
+# ensuring that pull requests are not merged with failing checks.
+#
+# Usage:
+# - Ensure that the job names in this workflow match exactly the names of the corresponding jobs in the main workflows.
+# - This workflow should be kept as-is, without path-specific conditions.
+
+name: no-op Checks
+on: pull_request
+
+jobs:
+ frontend-build:
+ runs-on: ubuntu-latest
+ steps:
+ - name: No-op for frontend-build
+ run: exit 0
+
Review Comment:
There are some inconsistent breaks between blocks if we want to get all OCD here.
I wonder: would it be a problem to add ALL required checks here? If so, I wonder if we could ingest and iterate through the list from asf.yaml rather than having to maintain this.
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27906:
URL: https://github.com/apache/superset/pull/27906#discussion_r1560082354
##########
.github/actions/setup-backend/action.yml:
##########
@@ -13,22 +13,39 @@ inputs:
description: 'Type of requirements to install. Options: base, development, default'
required: false
default: 'dev'
+ install-superset:
+ description: 'Whether to install Superset itself. If false, only python is installed'
+ required: false
+ default: 'true'
runs:
using: 'composite'
steps:
- - name: Set up Python ${{ inputs.python-version }}
+ - name: Interpret Python Version
+ id: set-python-version
+ shell: bash
+ run: |
+ if [ "${{ inputs.python-version }}" = "current" ]; then
+ echo "PYTHON_VERSION=3.10" >> $GITHUB_ENV
Review Comment:
OMG YES! But GHA doesn't play nicely with that kind of parameter injection. Composition through the "reusable action" approach that we have here is the best we can do with basic actions (calling a reusable action that hard-codes the values).
If we wanted more flexibility I'd suggest a Jinja2-based approach, where we'd have the source templates in say a `.github/jinja` folder, and render that into `.github/` via a commit hook or similar. Seemed overkill for now, but I'd be supportive to move in that direction.
Now comparing to where we were a few months back, we're at a much better place with this setup-backend and setup-supersetbot approach.
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "craig-rueda (via GitHub)" <gi...@apache.org>.
craig-rueda commented on code in PR #27906:
URL: https://github.com/apache/superset/pull/27906#discussion_r1554371108
##########
.github/workflows/no-op.yml:
##########
@@ -26,89 +26,103 @@ jobs:
frontend-build:
runs-on: ubuntu-latest
steps:
- - name: No-op for frontend-build
- run: |
- echo "This is a no-op step for frontend-build to ensure a successful status."
- exit 0
-
+ - run: exit 0
+# ------------------------------------
+# DE-COMMENT POST MERGING https://github.com/apache/superset/pull/27906
+# ------------------------------------
+#
+# pre-commit:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# python-lint:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# test-postgres-hive:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# test-postgres-presto:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# unit-tests:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# test-mysql:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# test-postgres:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+# test-sqlite:
+# runs-on: ubuntu-latest
+# steps:
+# - run: exit 0
+## ------------------------------------------------------
+## TO BE DEPRECATED after https://github.com/apache/superset/pull/27906
+## is merged
+## These no-ops were used to deprecate/renaming checks
+## and can be safely delete if seen on `master`
+## ------------------------------------------------------
pre-commit:
strategy:
matrix:
- python-version: ["3.9"]
+ python-version: ["3.9", "3.10", ""]
Review Comment:
Do we need to include `3.9`? Let's just do `3.10`
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #27906:
URL: https://github.com/apache/superset/pull/27906
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #27906:
URL: https://github.com/apache/superset/pull/27906#discussion_r1560023314
##########
.github/actions/setup-backend/action.yml:
##########
@@ -13,22 +13,39 @@ inputs:
description: 'Type of requirements to install. Options: base, development, default'
required: false
default: 'dev'
+ install-superset:
+ description: 'Whether to install Superset itself. If false, only python is installed'
+ required: false
+ default: 'true'
runs:
using: 'composite'
steps:
- - name: Set up Python ${{ inputs.python-version }}
+ - name: Interpret Python Version
+ id: set-python-version
+ shell: bash
+ run: |
+ if [ "${{ inputs.python-version }}" = "current" ]; then
+ echo "PYTHON_VERSION=3.10" >> $GITHUB_ENV
Review Comment:
No problem for this PR, but I'm thinking at some point we might want to keep our npm/node/python/whatever version numbers in a JSON file at the repo's root, so they can be pulled into scripts, documentation, etc. as needed.
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27906:
URL: https://github.com/apache/superset/pull/27906#issuecomment-2048103643
Capturing the conversation from the **Release Strategy Group** this morning:
- let's support a single python version officially per release, and run CI against that one mainly
- let's keep preventing "future regressions" on future python version by running some minimal CI against the future version of python (currently test-postgres against 3.11). For now this is the only place where we need a proper matrix. I might try to rename to "current" and "future" to address the required check issues
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "craig-rueda (via GitHub)" <gi...@apache.org>.
craig-rueda commented on PR #27906:
URL: https://github.com/apache/superset/pull/27906#issuecomment-2040708130
I'm all for just "officially" supporting a single PY version going forward. Most other projects do this AFAIK, so what's stopping us? :)
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "raphaelauv (via GitHub)" <gi...@apache.org>.
raphaelauv commented on PR #27906:
URL: https://github.com/apache/superset/pull/27906#issuecomment-2042253219
the only benefit of supporting multiple python version is to increase the chance to offer more compatible client/driver libraries for source data to the end users
example I use the database XXX , the python client is not compatible with python 3.11 , I want use latest version of Apache Superset version X.Y.Z that only support 3.11
a policy like supporting the last 2 version of python with active support could e a first step, wdyt ?
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27906:
URL: https://github.com/apache/superset/pull/27906#issuecomment-2043115346
Right, though we need to define what "support" means. Some criteria:
- providing an official Docker release for the version of python - probably running the CI/test suite at release time
- running all CI unit/integration tests on that version of python at all times, insuring that say `master` is compatible with that version of python at most times
- not doing pro-active tests, but also not restricting the python package letting people "run at their own risk" (this seems never desirable, and is kind of what we're doing currently)
What we're doing now:
- python package let's user install on 3.9, 3.10 and 3.11 I believe, based on the pyproject.toml
- we run the full test suite on 3.10
- we run one or two test suite on 3.11
- Official docker images are all 3.10
--
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] chore: [proposal] de-matrix python-version in GHAs [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27906:
URL: https://github.com/apache/superset/pull/27906#discussion_r1560016912
##########
.github/workflows/no-op.yml:
##########
@@ -0,0 +1,96 @@
+# no-op.yml
Review Comment:
I'll be able to delete this file after we merge this
--
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