Skip to content

Remove steps from travis#8383

Closed
dimberman wants to merge 21 commits intoapache:masterfrom
dimberman:remote-steps-from-travis
Closed

Remove steps from travis#8383
dimberman wants to merge 21 commits intoapache:masterfrom
dimberman:remote-steps-from-travis

Conversation

@dimberman
Copy link
Contributor

I hate travis. I hate it so much. Let's not add new things to
travis please.


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

I hate travis. I hate it so much. Let's not add new things to
travis please.
@turbaszek
Copy link
Member

We seem to be addressing different parts of the same issue #8376 :D

@dimberman
Copy link
Contributor Author

@turbaszek my PR for the migration script has been waiting for over an hour, so I am rage attacking travis with all of my might. Very down to collaborate but my sanity can't wait any longer

@ashb
Copy link
Member

ashb commented Apr 15, 2020

its-a-race-gif

(Github isn't display it, https://reneedezvous.files.wordpress.com/2015/12/its-a-race-gif.gif?w=490)

- name: install airflow
run: pip install -e .[devel]
- name: Prepare & test backport packages 1.10.9
run: ./scripts/ci/ci_prepare_and_test_backport_packages.sh
Copy link
Member

Choose a reason for hiding this comment

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

  - name: "Before install"
    run: pip install bowler
  - name: "Prepare backport packages"
    run: ./scripts/ci/ci_prepare_backport_packages.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@turbaszek I'm gonna try to get JUST static checks + dependency + docker and merge that to at minimum reduce pressure on travis.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to take some from my PR. Static checks, docs and test (without kube) should work

Copy link
Member

Choose a reason for hiding this comment

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

I had to run pre-commit run update-setup-cfg-file --all-files to make static checks work

runs-on: ubuntu-latest
strategy:
matrix:
python-version: [3.6,3.7]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python-version: [3.6,3.7]
python-version: [3.6, 3.7]

yamlint

uses: actions/setup-python@v1
with:
python-version: '3.7'
- name: install airflow
Copy link
Member

@potiuk potiuk Apr 15, 2020

Choose a reason for hiding this comment

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

Why this? We should not need to install airflow in the host - and it slows it all down a lot.

uses: actions/setup-python@v1
with:
python-version: '3.7'
- name: install airflow
Copy link
Member

@potiuk potiuk Apr 15, 2020

Choose a reason for hiding this comment

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

Same here. We do not need to install airflow on the host and it adds few minutes to the build

env:
PYTHON_VERSION: 3.6
AIRFLOW_MOUNT_SOURCE_DIR_FOR_STATIC_CHECKS: "true"
TRAVIS_JOB_NAME: "Static"
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather change condition in the script or maybe pass the name via run command parameter for example. The reason why I used name of the job was that I wanted to keep single "before_install" script. In Travis all the "before install" scripts are folded once finished so you do not see them in the logs by default. It would be great to find similar solution for Github Actions.

services:
- docker
jobs:
include:
Copy link
Member

@potiuk potiuk Apr 15, 2020

Choose a reason for hiding this comment

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

I am afraid we do not gain much by this. This will significantly INCREASE pressure on Travis. We have quite a lot of tests NOT running because static checks fail - that's probably >50% cases. If we just remove pre-test steps, then we will have a lot more "test" jobs blocking the Travis Queue because they will run regardless if static checks fail.

matrix:
scripts: [ci_run_static_checks_pylint_tests.sh,
ci_mypy.sh,
ci_fix_ownership.sh,
Copy link
Member

@potiuk potiuk Apr 15, 2020

Choose a reason for hiding this comment

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

Been there done that @dimberman .

I think this will not increase the speed. It might even slow it down.

There is quite an overhead introduced by creating a new runner (it works similarly in Travis and GA last time I checked). Vast majority of the tests - including mypy, llake8, bat_tests are fast and it makes no sense to spin separate worker just to run the single static check.. The ones that take vast majority (like 90%) of time are pylint_main and pylint_test), I experimented with it before and the best overall execution time was what was there before:

1, job - run all tests but pylint_tests
2, job - Run pylint_tests

Further fragmentation introduces much more overhead for worker creation and elapsed time to execute the builds was much longer. With this approach you can gain at most single seconds in the best case.

What would ultimately help if is we finish all pylint tests and introduce paralllelism in pylint tests (the tests can run muich faster but currently we have uncertainty of cycles introduced because some of our code is excluded from pylint and contains cycles, Those cycles are randomly appearing when people make their PRs so Pylint tests are now set to run without parallelism.

Copy link
Member

Choose a reason for hiding this comment

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

Also one more comment why this will be much, much, much slower - some of the tests (mypy, flake, pylint) require CI image to run (to keep consistent environment). If you split it to separate jobs, each of them will pull the image before running and will rebuild it with latest sources (to make sure they are using latest setup.py for tests). When you run mypy + flake + pylint in a single pre-commit run, they will all download the image and potentially build the image with the latest setup.py - this adds even more "per-job" overhead.

Copy link
Member

@turbaszek turbaszek Apr 15, 2020

Choose a reason for hiding this comment

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

I think what can help is to split static checks into action steps (should be easy with pre-commit). This will probably decrease performance but it will increase the readability of the output (you will see which step exactly failed).

Current output from statics checks is often a mess

Copy link
Member

Choose a reason for hiding this comment

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

I think what can help better is to keep most of the checks together but to get rid of the --verbose flag. The flag was introduced to see time of execution of each step. But... it's not needed any more in GA - in GA you can click the checkbox and see the timestamp for each line of the log. Once you disable the --verbose flag, it is much more obvious which step is failing because you will see a wall of green 'Passed" broken by some "Failed" (the output of each command is hidden if the result is "Passed"). This will be much better approach.

Copy link
Member

@turbaszek turbaszek Apr 15, 2020

Choose a reason for hiding this comment

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

At least we can move pylint and mypy to separate step. In this way the CI will fail faster if there are any other checks failing. And the feedback loop will be definitely faster.

@dimberman dimberman closed this Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants