Skip to content

Several tweaks to the CI and testing configurations #55

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 15, 2020
Merged

Several tweaks to the CI and testing configurations #55

merged 2 commits into from
Dec 15, 2020

Conversation

DilumAluthge
Copy link
Contributor

@DilumAluthge DilumAluthge commented Dec 12, 2020

Closes #39

This pull request makes the following modifications to the CI configuration:

  1. Disable Travis.
  2. Run GitHub Actions with both one and six threads.
  3. Separate the CI for Julia nightly into a separate workflow file. This is better than the current approach, which uses continue-on-error. With continue-on-error, it becomes very easy to fail to notice when your CI fails on Julia nightly. In contrast, the approach in this PR allows you to notice when CI fails on Julia nightly, while still having a green "Passing" CI badge in your README even if CI fails on Julia nightly.
  4. Add [compat] entries for all of the test-only dependencies.

@DilumAluthge DilumAluthge changed the title Several tweaks to the CI configuration Several tweaks to the CI and testing configurations Dec 12, 2020
@mcabbott
Copy link
Owner

This looks great, thank you! I meant to get around to 2, but clearly hadn't.

@codecov-io
Copy link

codecov-io commented Dec 12, 2020

Codecov Report

Merging #55 (0f9f0d8) into master (58c1fbb) will increase coverage by 1.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   79.98%   81.25%   +1.26%     
==========================================
  Files          18       18              
  Lines        1684     1686       +2     
==========================================
+ Hits         1347     1370      +23     
+ Misses        337      316      -21     
Impacted Files Coverage Δ
src/threads.jl 88.32% <0.00%> (+15.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58c1fbb...0f9f0d8. Read the comment docs.

@DilumAluthge
Copy link
Contributor Author

I'm going to change this to draft until I can get CI to pass.

@DilumAluthge DilumAluthge marked this pull request as draft December 12, 2020 12:40
@mcabbott
Copy link
Owner

There might be more fiddling around with VectorizationBase things required from my side, which didn't make it into #53.

@DilumAluthge
Copy link
Contributor Author

I think I have all the VectorizationBase stuff working now. Let's see if it passes on CI.

@DilumAluthge
Copy link
Contributor Author

There's a lot of "queued" jobs. You can go to https://github.com/mcabbott/Tullio.jl/actions?query=workflow%3ACI and https://github.com/mcabbott/Tullio.jl/actions?query=workflow%3A%22CI+%28Julia+nightly%29%22 and cancel all but the most recent job.

@chriselrod
Copy link
Contributor

FWIW, where there commented out Mask{<:Unsigned}s:

# Mask{UInt8}(0x02)

The proper constructor would be Mask{8}(0x02) to say it is a mask of size 8. One could reasonably want Mask{4}(0x02) or Mask{2}(0x02) instead, e.g. if one only has 256 bit or 128 bit vectors (Apple silicon only has 128 bit) with 64-bit types, or if a statically sized loop were <= 4 iterations.

@mcabbott
Copy link
Owner

Mask{UInt8}(0x02)

I was confused for a bit! Cleaned up a bit locally but... well now I made #57.

@DilumAluthge DilumAluthge marked this pull request as ready for review December 12, 2020 23:48
@DilumAluthge
Copy link
Contributor Author

This is good to merge.

Copy link
Owner

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

I would like to keep testing on 1.4, and LoopVectorization 0.8. But there's going to be some massaging to make this and #57 fit no matter what we do... so perhaps simplest to merge this & then work on top of it.

@DilumAluthge
Copy link
Contributor Author

I would like to keep testing on 1.4, and LoopVectorization 0.8.

Let me play around with this a bit and see if that's possible

@DilumAluthge DilumAluthge marked this pull request as draft December 14, 2020 00:24
@DilumAluthge
Copy link
Contributor Author

I would like to keep testing on 1.4, and LoopVectorization 0.8.

Alright, I've made some changes. We will keep running CI on Julia 1.4. On 1.4 CI we'll use LoopVectorization 0.8, and on 1.5 CI we'll use LoopVectorization 0.9.

I'll keep this as a draft until all of the CI jobs pass.

@DilumAluthge DilumAluthge marked this pull request as ready for review December 14, 2020 10:42
@DilumAluthge
Copy link
Contributor Author

@mcabbott Alright, take a look now. CI is passing on Julia 1.4 and Julia 1.5. So this should be ready to go.

We test on both Julia 1.4 and Julia 1.5. On Julia 1.4, we test with LoopVectorization 0.8, and on Julia 1.5 we test with LoopVectorization 0.9.

@mcabbott
Copy link
Owner

Thanks, this looks good, and well beyond the call of duty!

I have one last question, which I didn't notice earlier. All of them seem to show [ Info: Testing with 1 threads, which is odd. I wonder whether Github supports as many as 6, IIRC travis would only give you 2?

(6 is ideal for tests, as threader divides recursively by 3 or by 2 depending on how many threads remain. And when nthreads()>1, tests are run with a super-small block size to exercise this.)

@DilumAluthge
Copy link
Contributor Author

Thanks, this looks good, and well beyond the call of duty!

I have one last question, which I didn't notice earlier. All of them seem to show [ Info: Testing with 1 threads, which is odd. I wonder whether Github supports as many as 6, IIRC travis would only give you 2?

(6 is ideal for tests, as threader divides recursively by 3 or by 2 depending on how many threads remain. And when nthreads()>1, tests are run with a super-small block size to exercise this.)

Oof that's my mistake. I have a mistake in the workflow file.

@DilumAluthge
Copy link
Contributor Author

Yeah it looks like GitHub will also only give us two threads.

From https://github.com/mcabbott/Tullio.jl/pull/55/checks?check_run_id=1552071969

[ Info: Loading Tullio took 6.5 seconds
[ Info: Testing with 2 threads

@DilumAluthge
Copy link
Contributor Author

DilumAluthge commented Dec 15, 2020

But at least now the tests are running with different numbers of threads (one or two).

@mcabbott mcabbott merged commit 7e4eda9 into mcabbott:master Dec 15, 2020
@mcabbott mcabbott mentioned this pull request Dec 15, 2020
@DilumAluthge DilumAluthge deleted the dpa/disable-travis-ci branch December 15, 2020 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to travis-ci.com
4 participants