Skip to content

build: try to speed up Github workflows - #12090

Merged
ktmud merged 6 commits into
apache:masterfrom
ktmud:manage-github-workflow
Jan 7, 2021
Merged

build: try to speed up Github workflows#12090
ktmud merged 6 commits into
apache:masterfrom
ktmud:manage-github-workflow

Conversation

@ktmud

@ktmudktmud commented Dec 17, 2020

Copy link
Copy Markdown
Member

SUMMARY

CI has been slow recently. This PR is the latest effort in trying to mitigate:

  1. Update the Cancel Previous Run job added in build: cancel previous github action runs #11940 to cancel future jobs as well.

  2. Skip dependabot branches when possible (e.g. don't run Python jobs for npm updates).

  3. Merge lighter actions (Prefer Typescript and License Check) into one workflow (there are overheads in starting a job).

  4. Add a script to allow committers to manually cancel CI jobs in queue. This is useful on a busy day when the CI pipeline got clogged by continuous new commits to open PRs. Users have to configure GITHUB_TOKEN in their env variables in order to use this.

    (superset) ➜ incubator-superset git:(manage-github-workflow) ✗ ./scripts/cancel_github_workflows.py --help Usage: cancel_github_workflows.py [OPTIONS] BRANCH_OR_PULL
    Cancel running or queued GitHub workflows by branch or pull request ID.
    By default jobs that are already running or are the latest of the branch
    or PR will not be cancelled.
    Options:
    --repo TEXT Default is apache/incubator-superset
    --event [pull_request|push|issue]
    --include-last / --no-include-last
    Whether to also cancel the lastest run.
    --include-running / --no-include-running
    Whether to also cancel running workflows
    --help Show this message and exit.
    

    Snip20201204_53

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Test locally and CI

ADDITIONAL INFORMATION

  • Has associated issue: this is a rerun of build: try to speed up Github workflows #12089 because it was blocked by CI
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io

codecov-io commented Dec 17, 2020

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.22%. Comparing base (eb3c2b2) to head (85993fd).
⚠️ Report is 10942 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (eb3c2b2) and HEAD (85993fd). Click for more details.

HEAD has 7 uploads less than BASE
FlagBASE (eb3c2b2)HEAD (85993fd)
python104
javascript10
Additional details and impacted files
@@ Coverage Diff @@## master #12090 +/- ##
==========================================
- Coverage 67.14% 59.22% -7.93% 
==========================================
Files 1002 951 -51 Lines 49300 46658 -2642 Branches 5010 4312 -698 ==========================================
- Hits 33103 27633 -5470 - Misses 16072 19025 +2953 + Partials 125 0 -125 
FlagCoverage Δ
cypress51.44% <ø> (+<0.01%)⬆️
javascript?
python63.59% <ø> (-0.57%)⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ktmud
ktmudforce-pushed the manage-github-workflow branch from 16a2754 to c6ddbbfCompareDecember 21, 2020 07:47
@ktmud
ktmudforce-pushed the manage-github-workflow branch 2 times, most recently from 03494b0 to 1c4aba0CompareJanuary 4, 2021 18:36
@ktmud
ktmudforce-pushed the manage-github-workflow branch from 1c4aba0 to 3ca6ea6CompareJanuary 5, 2021 16:51
@ktmud
ktmudforce-pushed the manage-github-workflow branch from e8faa48 to 98e3922CompareJanuary 5, 2021 22:35
@ktmudktmud closed this Jan 5, 2021
@ktmudktmud reopened this Jan 5, 2021
@ktmud
ktmudforce-pushed the manage-github-workflow branch 3 times, most recently from 35c8d68 to 29df9f4CompareJanuary 6, 2021 00:06
Add a script to cancel previous GitHub workflows and optimize
the workflows in hope to speed up CI.
- Skip dependabot for python jobs
- Merge Prefer TypeScript and License Check
- Cancel all tests in one workflow
@ktmud
ktmudforce-pushed the manage-github-workflow branch from 29df9f4 to 85993fdCompareJanuary 6, 2021 20:54
continue-on-error: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is essentially useless since the comments will never work for PRs coming from forks (due to the GITHUB_TOKEN access). This would be a good candidate to port over to the workflow_run approach mentioned in https://securitylab.github.com/research/github-actions-preventing-pwn-requests. Obviously, out of scope for this PR

@villebrovillebro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few non-blocking comments, LGTM.

from typing import Iterable, List, Optional, Union

import click
import requests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever possible, we should probably try to use urllib3 over requests to avoid licensing issues.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it using Apache 2.0 License as well?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, they've apparently relicensed. There was a refactor effort some time ago to remove explicit dependency on requests as it wasn't Apache compatible back then (see #7643). Good to know.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I can't find any reference of it ever having been anything but Apache 2.0. Apparently I'm misremembering something or this has been a major misunderstanding.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't let this one go.. 😄 I read old mailing list archives and turned up this: https://lists.apache.org/thread.html/49c4d03d3aadab5c2d1c20ccf1cb7ca02edbc94961e6429dbf6a082b%40%3Cdev.superset.apache.org%3E Turns out requests was pulling in chardet which is LGPL, which was the problem. Turns out chardet was dropped as of 09/2020: encode/httpx#1269 🎉

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it means it's OK to use requests again, I guess?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhh, I went down a link in this issue without noticing that it was pointing to another project: psf/requests#4848 Apparently requests still uses chardet, and there are no plans to remove it. As long as this is the case we're not allowed to reference requests as a required dependency, but it's ok to be had as an optional dependency. As long as we restrict the usage to development scripts we're fine, but we shouldn't use it in the core code.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the clarification and investigation!

@@ -0,0 +1,141 @@
# Python MySQL unit tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly indifferent to having these split up versus all in one file, but I'd like to avoid shuffling these around unnecessarily to avoid losing the git history.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a hypothesis that there are overheads involved in scheduling multiple workflows vs running them in one. Not sure how much it helps but I still think it makes sense to combine these three considering they are very similar in nature and not particularly expensive to run.

@ktmud
ktmud merged commit f482849 into apache:masterJan 7, 2021
@ktmud
ktmud deleted the manage-github-workflow branch January 7, 2021 08:47
@mistercrunchmistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 First shipped in 1.0.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ botA label used by `supersetbot` to keep track of which PR where auto-tagged with release labelssize/XL🚢 1.0.0First shipped in 1.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ktmud@codecov-io@nytai@villebro@mistercrunch