Improve workflows to use github caches #182

zain-sohail · 2023-10-16T12:17:55Z

Generally addition of using caches in workflows
Parallization of testing of package and fixes for race conditions
Addition of dev dependencies so linting can be done

Refer to #151 for more details

Overall, workflows are much faster now
Linting: 1m
Test: 3-4m (after caching)

coveralls · 2023-10-16T12:24:40Z

Pull Request Test Coverage Report for Build 6656613697

43 of 51 (84.31%) changed or added relevant lines in 3 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 90.481%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tests/loader/test_loaders.py	25	33	75.76%

Files with Coverage Reduction	New Missed Lines	%
sed/calibrator/energy.py	2	91.86%
sed/core/processor.py	2	91.26%

Totals
Change from base Build 6655476886:	-0.2%
Covered Lines:	4268
Relevant Lines:	4717

💛 - Coveralls

rettigl · 2023-10-16T20:27:26Z

Coverage calculation does not work (or you added tests worth 9.1% coverage)

rettigl · 2023-10-16T20:29:38Z

Testing is not done on push, should be added for commits that do not belong to a PR yet

rettigl

In general, I think this is a nice addition, however I have a few concerns/suggestsions:

We need to make sure that the poetry.lock file is regularly updated to the latest dependency versions if we want to use this. Dependabot does not do this in its current config (see add depedabot for dependency tracking/updating #178)
There is currently no pytesting done on push. This should be added, at least for one python version, for pushes to branches without a PR.
Coverage reports do not seem to report correct numbers

.github/workflows/testing_multiversion.yml

rettigl · 2023-10-16T20:52:44Z

The pytest workflow does not even run when pushing to this branch/PR. Seems completely disabled right now.

daviddoji · 2023-10-17T06:40:49Z

@zain-sohail Related to this PR, if you also want to speed up on linting, have a look at ruff:
https://docs.astral.sh/ruff/

rettigl · 2023-10-17T08:27:51Z

@zain-sohail Related to this PR, if you also want to speed up on linting, have a look at ruff: https://docs.astral.sh/ruff/

Interesting, but it does not seem to replace pylint or mypy, which take the major time in our linting pipeline.
Pylint, however, can still be parallelized, -j parameter

zain-sohail · 2023-10-17T10:27:20Z

@zain-sohail Related to this PR, if you also want to speed up on linting, have a look at ruff: https://docs.astral.sh/ruff/

Interesting, but it does not seem to replace pylint or mypy, which take the major time in our linting pipeline. Pylint, however, can still be parallelized, -j parameter

I have thought about the Ruff option. If we can implement a similar rule set (or better) with Ruff, it's definitely faster. But we would still need a type checker like mypy.

zain-sohail · 2023-10-17T10:33:45Z

In general, I think this is a nice addition, however I have a few concerns/suggestsions:

We need to make sure that the poetry.lock file is regularly updated to the latest dependency versions if we want to use this. Dependabot does not do this in its current config (see add depedabot for dependency tracking/updating #178)

Addressed in that PR. We can implement this, if necessary, later.

There is currently no pytesting done on push. This should be added, at least for one python version, for pushes to branches without a PR.

But this is the same as before. We were never using push. See

sed/.github/workflows/pytest_coveralls.yml

Line 6 in 19921fc

on: pull_request

I just reimplemented the ruleset.

Coverage reports do not seem to report correct numbers

Yes, on that I am not quite certain why. Even for single version test, it doesn't. Or the older ones were wrong? Because now I am using the official action from coveralls.io
I will test the coverage locally and see.

daviddoji · 2023-10-17T10:43:11Z

@zain-sohail Related to this PR, if you also want to speed up on linting, have a look at ruff: https://docs.astral.sh/ruff/

Interesting, but it does not seem to replace pylint or mypy, which take the major time in our linting pipeline. Pylint, however, can still be parallelized, -j parameter

Ruff and mypy are tools for different purposes, while the former is a linter, the latter is a type checker. Each of them can detect problems that the other would miss. It's the combination of them what it makes sense to use.

Regarding pylint, maybe it's worth to give it a shot as many users have succeeded (https://docs.astral.sh/ruff/faq/#how-does-ruff-compare-to-pylint)

rettigl · 2023-10-17T12:01:10Z

3. There is currently no pytesting done on push. This should be added, at least for one python version, for pushes to branches without a PR.

But this is the same as before. We were never using push. See

sed/.github/workflows/pytest_coveralls.yml

Line 6 in 19921fc

on: pull_request

The tests were part of the pylint action (on push), your removed them from there. If you want to keep it that way, implement a new test action that runs on push.

Why the coveralls workflow does not run on push to the pull request branch, I don't know. Maybe "on" and "pull_request" need to be on the same line, as it was before

zain-sohail · 2023-10-17T12:07:19Z

There is currently no pytesting done on push. This should be added, at least for one python version, for pushes to branches without a PR.

But this is the same as before. We were never using push. See

sed/.github/workflows/pytest_coveralls.yml

Line 6 in 19921fc

on: pull_request

The tests were part of the pylint action (on push), your removed them from there. If you want to keep it that way, implement a new test action that runs on push.

Oh yes I removed them. That's a fair point. I will put the single version test to be on every push then?

Why the coveralls workflow does not run on push to the pull request branch, I don't know. Maybe "on" and "pull_request" need to be on the same line, as it was before

That shouldn't have mattered, from previous experience. I will check.

.github/workflows/testing.yml

tests/test_processor.py

zain-sohail · 2023-10-26T14:33:20Z

Is it good now? @rettigl

rettigl · 2023-10-26T14:59:24Z

Is it good now? @rettigl
Three more things:

rebase
add on: pull-request to the testing workflow
remove line 653 from test_processor

rettigl · 2023-10-26T15:00:29Z

Ah, and remove update_requirements

…nerated buffer files

…r.py

… coveralls

- added PR trigger - fixed unnecessary line in processor.py - changed testing file name to include coverage - removed update_requirements.yml

rettigl

LGTM now

zain-sohail · 2023-10-26T16:18:03Z

LGTM now

Thanks!

rettigl force-pushed the cached-workflows branch from c28e1c5 to 3097828 Compare October 16, 2023 20:41

rettigl reviewed Oct 16, 2023

View reviewed changes

.github/workflows/testing_multiversion.yml Outdated Show resolved Hide resolved

rettigl reviewed Oct 17, 2023

View reviewed changes

.github/workflows/testing.yml Outdated Show resolved Hide resolved

rettigl reviewed Oct 18, 2023

View reviewed changes

.github/workflows/testing.yml Outdated Show resolved Hide resolved

rettigl reviewed Oct 18, 2023

View reviewed changes

tests/test_processor.py Outdated Show resolved Hide resolved

zain-sohail and others added 11 commits October 26, 2023 17:06

add dev dependencies for linting

e8f05a7

update all workflows to use caches

e899b83

update all workflows to use caches

2354396

remove race condition for flash loader tests and remove additional ge…

2c1fed4

…nerated buffer files

remove race condition in test_io, and remove generated test files

50deecc

remove race condition for sed_config.yaml detetion from test_processo…

8d08a4e

…r.py

single version tests on every push, otherwise on push to main. fixing…

920abab

… coveralls

coveralls finish job

56f6b42

roll back to old usage

005770a

fix yaml error

ab4dce1

add an exact file and use with coveralls

5050752

- rebased

78e28f4

- added PR trigger - fixed unnecessary line in processor.py - changed testing file name to include coverage - removed update_requirements.yml

zain-sohail force-pushed the cached-workflows branch from f899127 to 78e28f4 Compare October 26, 2023 15:18

updated poetry.lock

bc38225

rettigl approved these changes Oct 26, 2023

View reviewed changes

zain-sohail merged commit 9e35e99 into main Oct 26, 2023
3 checks passed

zain-sohail deleted the cached-workflows branch October 26, 2023 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve workflows to use github caches #182

Improve workflows to use github caches #182

zain-sohail commented Oct 16, 2023 •

edited

Loading

coveralls commented Oct 16, 2023 •

edited

Loading

rettigl commented Oct 16, 2023 •

edited

Loading

rettigl commented Oct 16, 2023

rettigl left a comment

rettigl commented Oct 16, 2023

daviddoji commented Oct 17, 2023 •

edited

Loading

rettigl commented Oct 17, 2023 •

edited

Loading

zain-sohail commented Oct 17, 2023

zain-sohail commented Oct 17, 2023

daviddoji commented Oct 17, 2023

rettigl commented Oct 17, 2023

zain-sohail commented Oct 17, 2023

zain-sohail commented Oct 26, 2023

rettigl commented Oct 26, 2023

rettigl commented Oct 26, 2023

rettigl left a comment

zain-sohail commented Oct 26, 2023

Improve workflows to use github caches #182

Improve workflows to use github caches #182

Conversation

zain-sohail commented Oct 16, 2023 • edited Loading

coveralls commented Oct 16, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6656613697

💛 - Coveralls

rettigl commented Oct 16, 2023 • edited Loading

rettigl commented Oct 16, 2023

rettigl left a comment

Choose a reason for hiding this comment

rettigl commented Oct 16, 2023

daviddoji commented Oct 17, 2023 • edited Loading

rettigl commented Oct 17, 2023 • edited Loading

zain-sohail commented Oct 17, 2023

zain-sohail commented Oct 17, 2023

daviddoji commented Oct 17, 2023

rettigl commented Oct 17, 2023

zain-sohail commented Oct 17, 2023

zain-sohail commented Oct 26, 2023

rettigl commented Oct 26, 2023

rettigl commented Oct 26, 2023

rettigl left a comment

Choose a reason for hiding this comment

zain-sohail commented Oct 26, 2023

zain-sohail commented Oct 16, 2023 •

edited

Loading

coveralls commented Oct 16, 2023 •

edited

Loading

rettigl commented Oct 16, 2023 •

edited

Loading

daviddoji commented Oct 17, 2023 •

edited

Loading

rettigl commented Oct 17, 2023 •

edited

Loading