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/03/14 05:59:15 UTC
[PR] chore: 2nd try - simplify python dependencies [superset]
mistercrunch opened a new pull request, #27505:
URL: https://github.com/apache/superset/pull/27505
Keeping some of the good ideas from
https://github.com/apache/superset/pull/27404, but keeping pip-compile-multi
What's left here?
- keep pip-compile-multi, but bring things down to only base.txt and
development.txt
- reusable/simplified .github/actions/setup-backend/action.yml
--
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: 2nd try - simplify python dependencies [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #27505:
URL: https://github.com/apache/superset/pull/27505#discussion_r1525500016
##########
.github/workflows/superset-cli.yml:
##########
@@ -52,22 +52,8 @@ jobs:
continue-on-error: true
run: ./scripts/ci_check_no_file_changes.sh python
- name: Setup Python
+ uses: ./.github/actions/setup-backend/
if: steps.check.outcome == 'failure'
- uses: actions/setup-python@v5
- with:
- python-version: ${{ matrix.python-version }}
- cache: "pip"
- cache-dependency-path: "requirements/testing.txt"
- - name: Install dependencies
- if: steps.check.outcome == 'failure'
- uses: ./.github/actions/cached-dependencies
- with:
- run: |
- apt-get-install
- pip-upgrade
- pip install wheel
- pip install -r requirements/testing.txt
- setup-postgres
Review Comment:
do we still need this here?
##########
.github/workflows/superset-cli.yml:
##########
@@ -52,22 +52,8 @@ jobs:
continue-on-error: true
run: ./scripts/ci_check_no_file_changes.sh python
- name: Setup Python
+ uses: ./.github/actions/setup-backend/
if: steps.check.outcome == 'failure'
- uses: actions/setup-python@v5
- with:
- python-version: ${{ matrix.python-version }}
- cache: "pip"
- cache-dependency-path: "requirements/testing.txt"
- - name: Install dependencies
- if: steps.check.outcome == 'failure'
- uses: ./.github/actions/cached-dependencies
- with:
- run: |
- apt-get-install
- pip-upgrade
- pip install wheel
- pip install -r requirements/testing.txt
- setup-postgres
Review Comment:
do we still need `setup-postgres` here?
--
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: 2nd try - simplify python dependencies [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27505:
URL: https://github.com/apache/superset/pull/27505#issuecomment-1996593056
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27505?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 59.71%. Comparing base [(`fb39951`)](https://app.codecov.io/gh/apache/superset/commit/fb39951d2ae4be42c3d2cb4488b55978906b128b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`27c245a`)](https://app.codecov.io/gh/apache/superset/pull/27505?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #27505 +/- ##
==========================================
- Coverage 67.39% 59.71% -7.68%
==========================================
Files 1909 1909
Lines 74736 74709 -27
Branches 8326 8326
==========================================
- Hits 50367 44615 -5752
- Misses 22319 28044 +5725
Partials 2050 2050
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [mysql](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.70% <ø> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.39% <ø> (-15.90%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/27505/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.62% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27505?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: 2nd try - simplify python dependencies [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27505:
URL: https://github.com/apache/superset/pull/27505#discussion_r1525515067
##########
.github/workflows/superset-applitool-cypress.yml:
##########
@@ -58,23 +58,7 @@ jobs:
submodules: recursive
ref: master
- name: Setup Python
- uses: actions/setup-python@v5
- with:
- python-version: "3.9"
- - name: OS dependencies
- uses: ./.github/actions/cached-dependencies
- with:
- run: apt-get-install
- - name: Install python dependencies
- uses: ./.github/actions/cached-dependencies
- with:
- run: |
- pip-upgrade
- pip install -r requirements/testing.txt
- - name: Setup postgres
- uses: ./.github/actions/cached-dependencies
- with:
- run: setup-postgres
Review Comment:
oh good catch, I assumed we're good since CI is going through, but this is a job on a schedule
--
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: 2nd try - simplify python dependencies [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #27505:
URL: https://github.com/apache/superset/pull/27505#discussion_r1525500422
##########
.github/workflows/superset-applitool-cypress.yml:
##########
@@ -58,23 +58,7 @@ jobs:
submodules: recursive
ref: master
- name: Setup Python
- uses: actions/setup-python@v5
- with:
- python-version: "3.9"
- - name: OS dependencies
- uses: ./.github/actions/cached-dependencies
- with:
- run: apt-get-install
- - name: Install python dependencies
- uses: ./.github/actions/cached-dependencies
- with:
- run: |
- pip-upgrade
- pip install -r requirements/testing.txt
- - name: Setup postgres
- uses: ./.github/actions/cached-dependencies
- with:
- run: setup-postgres
Review Comment:
Do we still need `run: setup-postgres` here?
--
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: 2nd try - simplify python dependencies [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27505:
URL: https://github.com/apache/superset/pull/27505#issuecomment-1998676186
I don't think we should treat `requirements/*` as a public interface. Maybe, just maybe, we should treat `requirements/base.txt` as sensitive (renaming/deleting is would likely break some things somewhere).
I could be great to define the things or type of things we want to treat as a public interface for semver purposes and beyond. I'd argue that pinned deps shouldn't make that list.
Now with the pinned reqs keeping the package together that's a bit different, though that's why I'm trying to get back to a place where the pinned requirements would not be vital.
--
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: 2nd try - simplify python dependencies [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27505:
URL: https://github.com/apache/superset/pull/27505#issuecomment-1998648447
I added a note in UGPRADING.md, but would love to get @john-bodley 's blessing before merging this one
--
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: 2nd try - simplify python dependencies [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #27505:
URL: https://github.com/apache/superset/pull/27505#issuecomment-2005583598
Ok, merging this as I have other improvements lined up
--
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: 2nd try - simplify python dependencies [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #27505:
URL: https://github.com/apache/superset/pull/27505#issuecomment-1998660296
Great, or rather, even with the notes in UPGRADING.md, would this break semver if this is released in 4.1 for example?
--
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: 2nd try - simplify python dependencies [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #27505:
URL: https://github.com/apache/superset/pull/27505#discussion_r1525519909
##########
.github/workflows/superset-cli.yml:
##########
@@ -52,22 +52,8 @@ jobs:
continue-on-error: true
run: ./scripts/ci_check_no_file_changes.sh python
- name: Setup Python
+ uses: ./.github/actions/setup-backend/
if: steps.check.outcome == 'failure'
- uses: actions/setup-python@v5
- with:
- python-version: ${{ matrix.python-version }}
- cache: "pip"
- cache-dependency-path: "requirements/testing.txt"
- - name: Install dependencies
- if: steps.check.outcome == 'failure'
- uses: ./.github/actions/cached-dependencies
- with:
- run: |
- apt-get-install
- pip-upgrade
- pip install wheel
- pip install -r requirements/testing.txt
- setup-postgres
Review Comment:
mmmh, this one ran and succeedd without it. Looking at the code it seems semi-preventative stuff:
```
sudo apt-get update && sudo apt-get install --yes libecpg-dev
say "::group::Initialize database"
psql "postgresql://superset:superset@127.0.0.1:15432/superset" <<-EOF
DROP SCHEMA IF EXISTS sqllab_test_db CASCADE;
DROP SCHEMA IF EXISTS admin_database CASCADE;
CREATE SCHEMA sqllab_test_db;
CREATE SCHEMA admin_database;
```
I added it back just in case
--
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: 2nd try - simplify python dependencies [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #27505:
URL: https://github.com/apache/superset/pull/27505
--
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