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

Make LieAlgebraModule tests better gc-able #3913

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

lgoettgens
Copy link
Member

Adresses the memory issues in 1.11-nightly and nightly tests reported in #3831 (comment).

The excessive memory seems to come from the GAP iso adaption in #3831. Before this PR, the whole computation was done in GAP, the plan for the future is to not need GAP at all, but in the current state a lot of data has to be transferred (once per testcase).
If seems to help to run each testcase in its single scope to make everything garbage collectable right after the testcase ended (instead of currently keeping everything in memory until the @testset is over).
Furthermore I disabled the longest running testcase. The correctness of everything should still be covered by the other cases.

Let's hope that all of this helps with the CI failures.

@lgoettgens lgoettgens added topic: LieAlgebras experimental Only changes experimental parts of the code labels Jul 4, 2024
@lgoettgens lgoettgens requested a review from benlorenz July 4, 2024 14:53
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.99%. Comparing base (8d957f3) to head (125d206).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3913   +/-   ##
=======================================
  Coverage   83.99%   83.99%           
=======================================
  Files         591      591           
  Lines       81358    81380   +22     
=======================================
+ Hits        68333    68352   +19     
- Misses      13025    13028    +3     
Files Coverage Δ
...rimental/LieAlgebras/test/LieAlgebraModule-test.jl 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@benlorenz
Copy link
Member

Thanks for the quick improvements, memory usage is still quite large, but both jobs did succeed on the first try. I did a restart of the whole job to be sure.

For 1.11:

GC: pause 3754.52ms. collected 6522.264534MB. incr 
Heap stats: bytes_mapped 9474.31 MB, bytes_resident 9302.19 MB,
heap_size 9856.99 MB, heap_target 13168.92 MB, Fragmentation 0.295
Test Summary:                | Pass  Broken  Total     Time
LieAlgebras.LieAlgebraModule | 7431      49   7480  1m16.7s
-> Testing experimental/LieAlgebras/test/LieAlgebraModule-test.jl took: runtime 51.598 seconds + compilation 26.46 seconds + recompilation 0.0 seconds, 29.259 GiB

and nightly:

GC: pause 3664.34ms. collected 6586.752552MB. incr 
Heap stats: bytes_mapped 9602.34 MB, bytes_resident 9410.86 MB,
heap_size 9853.67 MB, heap_target 13062.80 MB, Fragmentation 0.296
Test Summary:                | Pass  Broken  Total     Time
LieAlgebras.LieAlgebraModule | 7431      49   7480  1m17.2s
-> Testing experimental/LieAlgebras/test/LieAlgebraModule-test.jl took: runtime 52.564 seconds + compilation 25.279 seconds + recompilation 0.721 seconds, 29.219 GiB

Previously this file had about 56 GiB of allocations when it did succeed.

Maybe we should also move this test-file to the long subset, which includes the more demanding tests and usually finishes first anyway. (Just add it to test_large in test/runtests.jl).

@lgoettgens
Copy link
Member Author

I'll wait for this test run to finish and then put the line into test_long. If all then three CI runs succeed without problems, this is good enough for me for the time being.

And I'll move the rewrite of the second part (to no longer need the GAP conversion) a bit up on my priority list.

Copy link
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

thanks!

@lgoettgens lgoettgens merged commit 953f8bf into oscar-system:master Jul 4, 2024
29 checks passed
@lgoettgens lgoettgens deleted the lg/LieModule-test-gc branch July 4, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Only changes experimental parts of the code topic: LieAlgebras
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants