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

Reduce runtimes for tests #1929

Open
HereAround opened this issue Feb 13, 2023 · 18 comments
Open

Reduce runtimes for tests #1929

HereAround opened this issue Feb 13, 2023 · 18 comments
Labels
optimization Simpler/more performant code or more/better tests testsuite Everything concerning testsuite

Comments

@HereAround
Copy link
Member

HereAround commented Feb 13, 2023

By now, the CI runs for OSCAR are arguably pretty long. Here are the timings from the last run in #1909:

As I see it, it would be great to reduce the runtimes (quicker to debug). Maybe we add this to the agenda on the OSCAR developer meeting, February 21 - 23? (@fieker , @wdecker)

Here are a few thoughts after a brief conversation with @fieker:

1. Optimizing the tests:

The code coverage should not be reduced and the executed tests should thoroughly check on the implemented methods. Still, maybe this could be achieved with fewer lines of code that are executed faster? As a starting point, one might want to have a list of the execution times, so that one can see which tests take up most of the time. For future investigation, this could possibly even be automatically updated on the OSCAR website say every 24 hours based on the latest runs with the latest OSCAR master?

2. More CIs:

In principle, one could envision to replace Run tests / test (1.6, x64, ubuntu-latest) by 4 CI runs, one for each cornerstone:

  • Run GAP tests / test (1.6, x64, ubuntu-latest),
  • Run Antic (Hecke, Nemo, Abstract Algebra) tests / test (1.6, x64, ubuntu-latest),
  • Run Polymake tests / test (1.6, x64, ubuntu-latest),
  • Run Singular & Algebraic Geometry tests / test (1.6, x64, ubuntu-latest).

So four machines would execute the tests. In an ideal world, thus only a quarter of the tests per machine. Realistically, the speed-up would not be factor 4, but one might still hope for a sizeable speedup. For instance, one of the four cornerstones probably takes longest for its tests (I have not checked, but my gut-feeling would be towards Singular & Algebraic Geometry). This real speed-up could be estimate based on an overview of the runtimes, as mentioned above.

An obstacle is that the number of parallel CI runs on github seems limited. I am not entirely sure if I read this information correctly, but if so then e.g. the free github plan limits this number to 20. In the case at hand, one would need 3 + 7 x 4 (i.e. keep the first three jobs in the list above and split the 7 other CI runs into 4 parallel runs each, one per cornerstone), i.e. 31 in total. And then, I am not sure if this limitation is per PR or for the entire repository. So for this to work efficiently, one might have to pay a sizeable amount for a suitable github account to host OSCAR.

3. Running the tests locally on a powerful machine:

As @fieker informs me, there seem to be thoughts on having a powerful machine which executes the test tests and then uploads the results to github.

Presumably, the above list is not complete and an ideal solution combines several distinct approaches.

@HereAround HereAround changed the title Reduce runtimes for CIs Reduce runtimes for tests Feb 13, 2023
@thofma
Copy link
Collaborator

thofma commented Feb 13, 2023

4. Run the tests in parallel

Although the github action runners are not that powerful, at least for macOS one can run tests them in parallel. There is nothing native in julia, but it is doable. It requires more structure in the organization of the tests, but it is worthwhile. We are doing this in Hecke, see for example https://github.com/thofma/Hecke.jl/blob/master/test/runtests.jl.

@joschmitt
Copy link
Member

See also #851 .

@benlorenz
Copy link
Member

The timeout for macos-nightly should be fixed, see #1888. (It is still rather slow though, last run on master was 84 minutes)

Regarding 1:
Currently it is somewhat tedious to get from the julia output of the test-timings to the corresponding source file as only the name of testset is printed but these names are not always that clear. Maybe we can prefix this with filenames or folder names automatically, then it would be easier to create such a list.

Regarding 2:
I don't think it makes sense like this, the Oscar tests do not tests the cornerstones separately. A lot of the code needs multiple cornerstones.
But we could probably split it up by subfolder (or group of subfolders) of the tests directory.

The oscar-system organization is currently in the Github Team plan (due to Max requesting some open-source discount), so we should have 60 concurrent jobs (But we are mostly limited by the at most 5 macOS jobs). Note that this count is for the whole oscar-system and not for each repository, so jobs running for e.g. Polymake.jl or Singular.jl may also delay Oscar.jl tests.

Regarding 4:
This might lead to memory exhaustion very quickly. We do have some quite demanding tests, see some of my posts in #1888.

5. Randomization:

We do have some testcases than can take very long but only from time to time due to randomization, this could be improved by using seeded random number generation. I have done this for one test-group and added some helpers to make it easier to use in this PR: #1459

@benlorenz
Copy link
Member

If anyone wants to look at some timings, I have created a branch that will print timings and allocations per file (not per testset) as a markdown table in the github job summary, e.g. https://github.com/oscar-system/Oscar.jl/actions/runs/4164703901 . (Using a slightly hacky modified Base.include wrapper...)

Example output:

Timings in seconds per file

Filename Time in s
Schemes/CoveredProjectiveSchemes.jl 150.314
./Experimental/GITFans-test.jl 136.535
./Geometry/K3Auto.jl 136.297
Serialization/PolynomialsSeries.jl 113.339
./Modules/UngradedModules.jl 95.0728
Rings/NumberField.jl 78.1349
./Experimental/gmodule-test.jl 76.8803
Schemes/ProjectiveSchemes.jl 69.9128
Rings/MPolyAnyMap/MPolyRing.jl 69.0524
Groups/spinor_norms.jl 68.7037
Groups/abelian_aut.jl 50.5808
./Experimental/MPolyRingSparse-test.jl 47.8758
Rings/binomial-ideals-test.jl 42.5651
Rings/AbelianClosure.jl 42.4565
Rings/mpoly-test.jl 41.2333

Allocations in megabytes per file

Filename Alloc in MB
Schemes/CoveredProjectiveSchemes.jl 35307.6
./Geometry/K3Auto.jl 34084.2
./Experimental/MPolyRingSparse-test.jl 12810.0
./Modules/UngradedModules.jl 10720.8
Rings/NumberField.jl 8743.1
Rings/binomial-ideals-test.jl 7692.84
Serialization/PolynomialsSeries.jl 7394.35
./Experimental/gmodule-test.jl 6625.31
Schemes/ProjectiveSchemes.jl 6056.55
./Experimental/galois-test.jl 4829.33
Groups/abelian_aut.jl 4704.07
Groups/spinor_norms.jl 4677.24
Rings/mpoly-graded-test.jl 4611.18
Schemes/AffineSchemes.jl 4558.54
Rings/MPolyAnyMap/MPolyRing.jl 4267.56

@thofma
Copy link
Collaborator

thofma commented Feb 13, 2023

Thanks! Would be great if we could hoax julia into not including compilation time for comparison.

@benlorenz
Copy link
Member

benlorenz commented Feb 13, 2023

Thanks! Would be great if we could hoax julia into not including compilation time for comparison.

Maybe if we do this as a separate job (via cron-trigger at night) and include every test-file twice (and use the timings for the second try)? (Edit: that is probably not ideal due to caching)

Edit:
We might be able to convince julia to produce compilation times separately but extracting this needs to be done manually as the @timed macro doesn't return it (and the code to do this depends on the minor julia version). Also it seems that this feature is incompatible with code-coverage so this should probably be a separate job with just very few configurations.

@benlorenz
Copy link
Member

Thanks! Would be great if we could hoax julia into not including compilation time for comparison.

This does seem to work now, see for example this table: https://github.com/oscar-system/Oscar.jl/actions/runs/4175058172#summary-11332566695

I have created a draft PR for this: #1942

@HereAround
Copy link
Member Author

Wow. This really looks great!

@thofma
Copy link
Collaborator

thofma commented Feb 15, 2023

Indeed, great! I was also wondering why the @timed macro does not include compilation time, although it is prined for @time.

@lgoettgens
Copy link
Member

Since the merge of 10100da, the execution time of the doctests in julia 1.6 on ubuntu went up from 15-20 minutes (cf. here) to > 30 minutes after (cf. here).
This has led to timeouts e.g. here and here.

Would it be possible to split the doctests into their own job to circumvent the 120 minutes deadline?

@lkastner
Copy link
Member

Would it be possible to split the doctests into their own job to circumvent the 120 minutes deadline?

The timeout is set by us in #1226. Nevertheless we should think about how to manage this. The testsuite will continue to grow in the future and at the current rate we will exhaust the limit of github actions pretty soon, I think this is at 5 hours. I'd propose to subdivide the testsuite into a "core" and "extended", if that is possible in a reasonable way. Then these two things could run separately.

But that the doctests went up by 10min seems a bit excessive. What are your thoughts about the difference between doctests vs tests? For me stuff in the doctests should be small and basically only illustrate usage. It is only tested to make sure that nothing in the docs is broken. Things requiring performance belong should be tests. But maybe that is only my opinion? At some point we also had gigantic ideals in the tropical geometry doctests and I'd say this issue is similar.

@HereAround
Copy link
Member Author

Since the merge of 10100da, the execution time of the doctests in julia 1.6 on ubuntu went up from 15-20 minutes (cf. here) to > 30 minutes after (cf. here). This has led to timeouts e.g. here and here.

Would it be possible to split the doctests into their own job to circumvent the 120 minutes deadline?

I was (and am) aware that the doctests for FTheoryTools would extend the runtime for the OSCAR doctests. However, as of my latest knowledge the OSCAR doctests took about 30 minutes while e.g. the CI-tests often run for about 60 to 90 (and sometimes even close to 120) minutes. So I did not see a need to speed-up the doctests (yet).

Let me mention that I did deactive CI tests for FTheoryTools that would run for about 20 minutes. For once, those are definitely too long (could be made into an example of tutorial), and because that would indeed lead to timeouts of the OSCAR CI tests. As @lkastner says, currently, the timeout is set by hand in the testsuite to 2 hours. To my knowledge, github limits the runtime for most repositories to 6 hours. As @benlorenz said above, OSCAR is in the Github Team plan, and so might have a bit more resources.

Either way - could you explain why the longer doctests caused https://github.com/oscar-system/Oscar.jl/actions/runs/4553825159/jobs/8030881380 and https://github.com/oscar-system/Oscar.jl/actions/runs/4556618792/jobs/8037205840? I was not aware that there is a correlation among the CI tests in julia version 1.6 and the doctests.

@HereAround
Copy link
Member Author

Let me investigate why the FTheoryTool doctests take so long. They should indeed be quicker...

@lgoettgens
Copy link
Member

Either way - could you explain why the longer doctests caused https://github.com/oscar-system/Oscar.jl/actions/runs/4553825159/jobs/8030881380 and https://github.com/oscar-system/Oscar.jl/actions/runs/4556618792/jobs/8037205840? I was not aware that there is a correlation among the CI tests in julia version 1.6 and the doctests.

The doctests are run as part of the testsuite iff the julIaversion is 1.6. When you turn on timestamps in the logs, you can see that here the doctests need > 30 minutes.
In particular, the first failing pr one did not introduce any new other tests.

@HereAround
Copy link
Member Author

HereAround commented Mar 30, 2023

Ah. I see. Sorry I was not aware of this. I will think of something to speed-up the doctests for FTheoryTools. Thank you for pointing this out!

@HereAround
Copy link
Member Author

Thinking of it - is it really necessary to run the doctests as part of the 1.6 CI-tests. We run

Since the merge of 10100da, the execution time of the doctests in julia 1.6 on ubuntu went up from 15-20 minutes (cf. here) to > 30 minutes after (cf. here). This has led to timeouts e.g. here and here.

Would it be possible to split the doctests into their own job to circumvent the 120 minutes deadline?

I never looked at it before, but indeed assumed that the github action documentation would run the doctests and then deploy the documentation. Along your suggestion, maybe this would be the natural place to execute the doctests, rather than running them once the 1.6 CI tests completed.

(But still, the point remains that the doctests for the FTheoryTools take too long. I will work on a speed-up for them.)

@thofma
Copy link
Collaborator

thofma commented Mar 30, 2023

#1768 added the doctests to the normal CI.

The main reason is that it makes them count for coverage.

@lkastner
Copy link
Member

lkastner commented Apr 2, 2023

As another aspect I want to mention the comments in #2187.

It seems for Julia's Nanosoldier the package tests are limited to 45 minutes. As a result, Oscar is currently failing Nanosoldier testing anyway.

So I propose to subdivide the tests into three categories: nanosoldier, core, and extended. Whatever we do, we should have some testset that fits into the nanosoldier thingy and that isn't touched by the average Lars by accident.

@lkastner lkastner added testsuite Everything concerning testsuite optimization Simpler/more performant code or more/better tests labels Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Simpler/more performant code or more/better tests testsuite Everything concerning testsuite
Projects
None yet
Development

No branches or pull requests

6 participants