Skip to content
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

allow stdlibs to run their tests in parallel #25778

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Jan 27, 2018

After some of the larger modules (like LinearAlgebra) was moved to stdlib it became evident that having each stdlib run its tests in serial is a bit painful.
In addition, it is not possible to choose to only run a part of a stdlibs test (for example the matmul tests in LinearAlgebra).
There have also been some concerns about the memory usage of e.g. LinearAlgebra when it is tied to one worker.
This PR presents a remedy to this problem. Each stdlib can optionally chose to have a file tests in their test folder. This is a list of testnames, each corresponding to a file that can be run independently. If this file does not exist, we fall back to just using runtests.jl which is supposed to run the tests serially. We can run a specific test for a stdlib using e.g. LinearAlgebra/matmul.

I have here chosen only to create test files for LinearAlgebra and SparseArrays because these tend to take a significant time. Not sure how worth it is to do the same for other stdlibs.

Below is the result of running ./julia test/runtests.jl SparseArrays:

Test (Worker)                   | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
SparseArrays/higherorderfns (2) |  131.55  |  1.65  |  1.3 | 4719.44    | 389.58
SparseArrays/sparse (3)         |  141.43  | 14.61  | 10.3 | 4530.10    | 408.35
SparseArrays/sparsevector (4)   |  199.48  |  3.58  |  1.8 | 10503.28   | 649.96

Test Summary: |  Pass  Broken  Total
  Overall     | 18317     584  18901
    SUCCESS

And this is ./julia test/runtests.jl LinearAlgebra/lq:

Test (Worker)        | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
LinearAlgebra/lq (1) |   61.69  |  1.25  |  2.0 | 5482.55    | 397.44

Test Summary: | Pass  Total
  Overall     | 1285   1285
    SUCCESS

In the test output, it will write e.g. REPL/runtests now, if the stdlib falls back to use runtests.jl. I thought about filtering the testname if the fallback file is used but didn't know how much it mattered. Edit: Fixed

cc @mbauman, @andreasnoack

This might lead to slightly longer CI times because it undoes the effect mentioned in #25571 (comment). I will probably make a follow up PR to this one where each worker is encouraged to work as much as it can on each testgroup instead of just pulling the next one in the list.

@KristofferC KristofferC added the testsystem The unit testing framework and Test stdlib label Jan 27, 2018
@JeffBezanson
Copy link
Member

Nice! Maybe call the tests file testnames --- since it contains names and not tests themselves, and since that's the term you used to explain what it is :)

@KristofferC
Copy link
Member Author

I think I will call it testgroups.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jan 27, 2018

It's true that testnames sounds like the names of individual tests so +1 testgroups. I wonder if this file name should be capitalized or something, i.e. TEST_GROUPS or maybe have an extension like testgroups.csv or even test_groups.csv?

@StefanKarpinski
Copy link
Member

Actually, since we're using TOML for all of the package configuration, maybe there should be a Tests.toml file that has a [groups] section or entry, e.g.

# test/Tests.toml
groups = [
  "triangular",
  "qr",
  "dense",
  ...
]

@KristofferC KristofferC force-pushed the the_return_of_the_parallel_stdlib_tests branch from dd07d50 to 7f91024 Compare January 28, 2018 12:23
@KristofferC
Copy link
Member Author

KristofferC commented Jan 28, 2018

Changed the name to testgroups and made the /runtests no longer print. Changing the file format to .TOML and adding groups keys etc gives the impression that there exist more configuration choices than currently possible. I don't think this is the best way to configure tests but it is simple and should be good for now. Also added testgroups for Dates.

As a bonus I made the printing a bit (imo) better. The worker number now align to the right and all other numbers like time and allocations etc also align to the right since that is usually better for numbers:

Test      (Worker) | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
Base64         (3) |     2.57 |   0.04 |  1.4 |     106.62 |   174.24
CRC32c         (2) |     3.21 |   0.10 |  3.1 |     123.89 |   182.39
strings/search (5) |     3.76 |   0.10 |  2.5 |     121.99 |   184.30
strings/types  (8) |     4.15 |   0.11 |  2.6 |     153.65 |   185.70
strings/io     (7) |     4.36 |   0.10 |  2.4 |     133.33 |   191.80
strings/util   (6) |     4.94 |   0.10 |  1.9 |     114.41 |   197.00
strings/basic  (4) |    11.39 |   0.20 |  1.7 |     389.01 |   210.68

this is done by creating a file `tests` in the `test` folder
that lists the different files with tests (excluding ".jl").
If this file does not exist, we fall back to using the runtests.jl file
@KristofferC KristofferC force-pushed the the_return_of_the_parallel_stdlib_tests branch from 7f91024 to e2919d9 Compare January 28, 2018 12:50
@KristofferC
Copy link
Member Author

KristofferC commented Jan 28, 2018

The test groups with the largest times are now:

libgit2 (53)                      |  342.81  |  2.06  |  0.6 | 955.95     | 451.29  
subarray (50)                     |  436.33  | 21.30  |  4.9 | 13940.37   | 863.25  
ranges (64)                       |  501.31  | 25.68  |  5.1 | 14934.81   | 484.01  
LinearAlgebra/triangular (6)      |  743.36  | 33.17  |  4.5 | 19967.33   | 1171.40 

Splitting these up would likely not make much difference to CI time but for developers who have access to high core machines, it will likely make running the full suite faster.

And as a remark, in the triangular tests:

for elty1 in (Float32, Float64, BigFloat, ComplexF32, ComplexF64, Complex{BigFloat}, Int)
    # Begin loop for first Triangular matrix
    for (t1, uplo1) in ((UpperTriangular, :U),
                        (UnitUpperTriangular, :U),
                        (LowerTriangular, :L),
                        (UnitLowerTriangular, :L))

Can that really be completely necessary...?

@Sacha0
Copy link
Member

Sacha0 commented Jan 28, 2018

Can that really be completely necessary...?

Which part? :)

@KristofferC
Copy link
Member Author

The part that takes over ten minutes to test. Brute forcing is fine if the running time doesn't become is excessive. This is very excessive in my opinion. Should be possible to do it in a more clever way.

@KristofferC
Copy link
Member Author

Should be good to go.

include("io.jl")
include("arithmetic.jl")
include("conversions.jl")
for file in readlines(joinpath(@__DIR__, "testgroups")
Copy link
Member

@JeffBezanson JeffBezanson Jan 29, 2018

Choose a reason for hiding this comment

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

Missing a close paren. (also in the other runtests.jl files)

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops! Fixed. Pushed with ci skipped because these files don't get run during CI anyway.

@andreasnoack
Copy link
Member

andreasnoack commented Jan 29, 2018

Should be possible to do it in a more clever way.

Maybe, but it is hard to predict where the promotion/dispatch issues could occur. It is more often the case that a code change introduces unintended dispatch changes than it introduces an error is in an algorithm. Reducing coverage is an easy way to reduce test time. The time here is not spent on repeated calls to the same methods. Almost all of the time is spent on compiling methods so speeding up the tests is the same as not testing these methods.

If compilation time is a serious concern, a more fundamental solution would be to restrict two-argument in-place linear algebra methods to the same element types. I.e continue to support rand(Float64, 2, 2)*rand(Float32, 2, 2) but not mul!(Matrix{Float64}(2,2), rand(Float64, 2, 2), rand(Float32, 2, 2)). That would probably have a huge impact since it is the repeated compilation of large methods like generic_matmatmul! that is costly whereas promotion methods are cheap.

@KristofferC KristofferC merged commit 8d8f960 into master Jan 29, 2018
@KristofferC KristofferC deleted the the_return_of_the_parallel_stdlib_tests branch January 29, 2018 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants