Load hulpke/extraperfect as lazy artifact
#2432
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2432 +/- ##
==========================================
- Coverage 73.15% 73.12% -0.03%
==========================================
Files 400 401 +1
Lines 53607 53623 +16
==========================================
- Hits 39215 39213 -2
- Misses 14392 14410 +18
|
lgoettgens
force-pushed
the
lg/perfgrp
branch
2 times, most recently
from
c80e3a2 to
338c5c0
Compare
There was a problem hiding this comment.
Great, this is looking pretty nice now! I have one minor concern about the test which includes network access (CC @ThomasBreuer @thofma @benlorenz )
test/Groups/libraries.jl
Outdated
| @@ -108,6 +108,9 @@ end | |||
| @test number_perfect_groups(ZZRingElem(60)^3) == 1 | |||
| @test_throws ArgumentError number_perfect_groups(0) # invalid argument | |||
| @test_throws ArgumentError number_perfect_groups(ZZRingElem(60)^10) # result not known | |||
|
|
|||
| # lazy artifact loading | |||
| @test perfect_group(1376256, 1) isa PermGroup | |||
There was a problem hiding this comment.
Hmm, that adds (another?) network access to the test suite, as such the tests may fail if one is offline. And of course it increases disk usage for the testing machine (which is fine for me locally, it's CI jobs I am very slightly worried about).
Dunno if this is a problem with e.g. NanoSoldier.
Anyway, one way to try to deal with this might be to modify this to catch whatever exception is thrown when a download fails, and just accept that?
There was a problem hiding this comment.
Would it make sense to introduce a global offline flag for the tests, meaning that if the flag is set to true then all those tests that (may) require network access are not executed?
Then we would get an overview of those situations that are expected to require network access, and we can exclude such tests from the CI runs.
There was a problem hiding this comment.
I could imagine 3 ways how to handle this
- Add some global bool
is_offlineto the tests and replace the test byis_offline || @test ... - Add a macro
@test_if_onlinethat decides based on either a global bool or some heuristic (e.g. test network access by pinging something) if the test gets executed or not. - Add a macro
@test_onlinethat tries to run the tests and catches whatever exceptions may occur while using the network, and does not fail in the case of such an exception.
Names are of course open to discussion. What would be your preferred way?
There was a problem hiding this comment.
I am open to all. Let's ask @thofma @fieker @benlorenz if they have thoughts on this, too?
There was a problem hiding this comment.
I lean towards 3. (with a @warning or @info when the exceptions occur), but have no strong opinion.
Although things will get messy once people start writing (and testing) functions, which might require indirectly network access. It is not obvious to a caller which function might have this problem.
There was a problem hiding this comment.
Whatever way this is implemented, I would prefer not to run this by default for users testing this (even with a network check, I might be using a limited mobile hotspot since my office doesn't have WiFi...). For github actions CI we may run this but I think this should not fail the CI for a pull request.
But this leads to the question when would this really run and how would it be noticed if it fails? Another separate job which will only run the network dependent jobs and is allowed to fail?
There is the possibility to set a warning in github actions, this would add a small icon to the job: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-warning-message
I think the NanoSoldier environment does allow network access, but again I would prefer if it does not fail due to network issues.
There was a problem hiding this comment.
In order to not let this patch bit-rot, I suggest we comment this test out for now, file an issue to remind us about the possibility for optional online tests.
test/Groups/libraries.jl
Outdated
| @@ -108,6 +108,9 @@ end | |||
| @test number_perfect_groups(ZZRingElem(60)^3) == 1 | |||
| @test_throws ArgumentError number_perfect_groups(0) # invalid argument | |||
| @test_throws ArgumentError number_perfect_groups(ZZRingElem(60)^10) # result not known | |||
|
|
|||
| # lazy artifact loading | |||
| @test perfect_group(1376256, 1) isa PermGroup | |||
There was a problem hiding this comment.
Whatever way this is implemented, I would prefer not to run this by default for users testing this (even with a network check, I might be using a limited mobile hotspot since my office doesn't have WiFi...). For github actions CI we may run this but I think this should not fail the CI for a pull request.
But this leads to the question when would this really run and how would it be noticed if it fails? Another separate job which will only run the network dependent jobs and is allowed to fail?
There is the possibility to set a warning in github actions, this would add a small icon to the job: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-warning-message
I think the NanoSoldier environment does allow network access, but again I would prefer if it does not fail due to network issues.
to circumvent the additional arg only being present since julia 1.9
Co-authored-by: Benjamin Lorenz <benlorenz@users.noreply.github.com>
|
I updated the artifact location, included the generation script as a comment in the |
|
nightly failure is: oscar-system/GAP.jl#901 / #2476 |
Resolves #1245.
Hook into
GAP'sPerfGrpLoadto create a symlink to a lazy artifact instead of throwing the error shown in #1245.lazy = truefor an artifact needsLazyArtifactsto be imported, and only downloads the artifact the first time the artifact string (theartifact"gap_hulpke_extraperfect") is encountered.I used a fixed commit of hulpke/extraperfect instead of
masterto avoid hash mismatches once there is a new commit onhulpke/extraperfect#master. This means that the used version ofhulpke/extraperfecthas to be actively updated inArtifacts.toml.