rewrite `julia_to_gap` #1029

ThomasBreuer · 2024-08-30T09:23:42Z

This is a first attempt to issue #1025.
The ideas are the same as in pull request #777, which deals with "the other direction", that is, the conversion from GAP to Julia:

Make the recursive conversion safe. Up to now, we have ad-hoc delegation from julia_to_gap methods with three arguments back to a julia_to_gap with only one argument.
Make the conversion more efficient. Up to now, a dictionary for tracking subobjects is created by default in the methods in question, also in situations where this dictionary is not used later on.
Make the installation of new conversion methods simpler. Up to now it happens (for example in Oscar.jl) that the installation of a GapObj method for some Oscar objects does not work as expected for vectors of these objects, or that changes in GAP.jl break the intended behaviour of GapObj methods in Oscar.jl (see GapObj vs. julia_to_gap #1025).

The conversion from Julia to GAP is simpler than that from GAP to Julia for example because we need not care about caching different Julia target types for the same GAP object. On the other hand, the direction from Julia to GAP shall cover also the constructor GapObj in situations where one wants to fetch an existing GAP object stored in a Julia object.

The current changes mean that all Julia to GAP conversions have to be installed via GapObj_internal methods that have three arguments.
Then GapObj needs only default methods that delegate to GapObj_internal.

If we proceed like this then these changes are breaking.
For Oscar.jl, they mean that the julia_to_gap and GapObj methods installed there have to be rewritten.
(Currently the master branch of Oscar.jl has 23 julia_to_gap methods, half of them in src/GAP/oscar_to_gap.)

Here are the technical details.

introduce GapObj_internal, which takes always three arguments: the Julia object to be converted, the auxiliary cache object, and a Boolean indicating whether recursive conversion is wanted
turn all julia_to_gap methods into GapObj_internal methods
define default GapObj methods (with 1 to 3 arguments) that call GapObj_internal
admit nothing as an allowed value for the auxiliary cache object, and create dictionary objects only inside the methods in question where they are needed; introduce the type GapCacheDict for that
provide a macro for the installation of unary GapObj methods (i.e., methods that do not require the recursive tracking of subobjects w.r.t. object identity) via the syntax GAP.@install GapObj(x::T) = f(x)

codecov · 2024-08-30T09:28:26Z

Codecov Report

Attention: Patch coverage is 95.08197% with 6 lines in your changes missing coverage. Please review.

Project coverage is 75.81%. Comparing base (e60382d) to head (d7ee6f9).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/JuliaInterface/gap/convert.gi	50.00%	3 Missing ⚠️
src/macros.jl	91.30%	2 Missing ⚠️
src/julia_to_gap.jl	98.82%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1029      +/-   ##
==========================================
+ Coverage   75.61%   75.81%   +0.19%     
==========================================
  Files          55       55              
  Lines        4478     4531      +53     
==========================================
+ Hits         3386     3435      +49     
- Misses       1092     1096       +4

Files with missing lines	Coverage Δ
pkg/JuliaInterface/gap/JuliaInterface.gd	`100.00% <ø> (ø)`
pkg/JuliaInterface/gap/convert.gd	`100.00% <ø> (ø)`
pkg/JuliaInterface/gap/juliahelp.g	`90.38% <ø> (ø)`
src/adapter.jl	`71.28% <ø> (ø)`
src/ccalls.jl	`99.19% <ø> (ø)`
src/constructors.jl	`98.71% <100.00%> (-0.02%)`	⬇️
src/gap_to_julia.jl	`89.78% <ø> (ø)`
src/types.jl	`80.00% <ø> (ø)`
src/wrappers.jl	`98.61% <100.00%> (+0.06%)`	⬆️
src/julia_to_gap.jl	`98.49% <98.82%> (+2.37%)`	⬆️
... and 2 more

... and 1 file with indirect coverage changes

ThomasBreuer · 2024-08-30T10:30:24Z

@fingolfin If it is of interest:
With GAP.jl as given by this pull request, I get reproducible segmentation faults on my notebook when I enter the following in Julia 1.10.4.

using GAP
using Test
include("packages/GAP.jl/test/runtests.jl")

The log looks similar to the one from the failed CI tests on github (Julia 1.10 - ubuntu-latest), but I get the crash a bit later: The GAP 4.13.1 can load its packages, processes a few testfiles, and crashes in GAP.jl/pkg/JuliaInterface/tst/help.tst.

test/macros.jl

src/macros.jl

src/julia_to_gap.jl

fingolfin · 2024-09-04T00:24:58Z

src/julia_to_gap.jl

        recursion_dict[obj] = ret_val
    end
    for i = 1:rows
-        ret_val[i] = julia_to_gap(obj[i, :], recursion_dict; recursive)
+        ret_val[i] = julia_to_gap_internal(obj[i, :], recursion_dict, recursive)


Existing issue, but shouldn't this also do the get!(recursion_dict::RecDict, x) dance?

I think no.
The idea of the get! call is twofold.

Check whether the object in question is already in the dictionary, and if yes then take the value.

If the object is not yet in the dictionary then store it together with the converted value.
Situation 1. cannot occur because we are just now creating the Julia object anew.
And situation 2. would not help later on because no pointer to this newly created object can be found in the recursion.
(I will add a comment.)

I feel there is something inconsistent here. Because then the object produced by recurively called julia_to_gap_internal is potentially never added to recursion_dict, is it? So if we had T = BigInt we would not be caching the conversion results for numerator or denominator. While if we convert a list of BigInt they would be cached.

Perhaps we never want to cache a BigInt's converted while. But then why does the julia_to_gap_internal method for Rational{T} provide the the full 3-argument form instead of just using GAP.@install?

OK the crucial point I missed is that we convert row-wise, i.e. we convert obj[i, :] and not elementwise, i.e. obj[i,j]

fingolfin · 2024-09-04T00:27:15Z

src/julia_to_gap.jl

        recursion_dict[obj] = ret_val
        for i = 1:len
-             ret_val[i] = julia_to_gap(obj[i], recursion_dict; recursive)
+             ret_val[i] = julia_to_gap_internal(obj[i], recursion_dict, recursive)


Another case with missing get!(recursion_dict::RecDict, x) etc.?

Yes, good spot!
I will add a test for this situation (a GAP list containing identical Julia objects which themselves do not support/need recursion).

fingolfin · 2024-09-04T11:58:23Z

src/julia_to_gap.jl

+    recursive::Bool,
 ) where {T}


We could enhance the non-recursive case by using a Val type instead:

Suggested change

recursive::Bool,

) where {T}

::Val{recursive},

) where {T, recursive}

This way the Julia compiler can produce seperate optimized code for both recursive = true and recursive = false.

fingolfin · 2024-09-04T11:59:13Z

src/julia_to_gap.jl

 ) where {T}

+    if recursion_dict !== nothing && haskey(recursion_dict, obj)


Hmm, I wonder if this should be

Suggested change

if recursion_dict !== nothing && haskey(recursion_dict, obj)

if recursion && recursion_dict !== nothing && haskey(recursion_dict, obj)

That is, I think the invariant "recursion_dict !== nothing implies recursion == true" should hold.

So this change normally shouldn't matter, but if we also change recursion to use a Val type then this will allow the compiler to elide this entire if block if recursion == false holds

fingolfin · 2024-09-04T12:05:26Z

src/julia_to_gap.jl

        recursion_dict[obj] = ret_val
    end
    for i = 1:rows
-        ret_val[i] = julia_to_gap(obj[i, :], recursion_dict; recursive)
+        ret_val[i] = julia_to_gap_internal(obj[i, :], recursion_dict, recursive)


I feel there is something inconsistent here. Because then the object produced by recurively called julia_to_gap_internal is potentially never added to recursion_dict, is it? So if we had T = BigInt we would not be caching the conversion results for numerator or denominator. While if we convert a list of BigInt they would be cached.

Perhaps we never want to cache a BigInt's converted while. But then why does the julia_to_gap_internal method for Rational{T} provide the the full 3-argument form instead of just using GAP.@install?

src/gap_to_julia.jl

fingolfin · 2024-09-04T13:29:31Z

src/julia_to_gap.jl

@@ -132,79 +140,83 @@ function julia_to_gap(
            continue
        end
        if recursive
-            x = get!(recursion_dict, x) do
-                julia_to_gap(x, recursion_dict; recursive)
+            res = get!(recursion_dict::RecDict, x) do


Just to record some of my comments from our oral discussion earlier for posterity:

We should remove the get! calls here and instead leave to each julia_to_gap_internal method whether to use it (or not).

The idea here is: for e.g. Int16 we never want to use the cache. But for a more complex object we might.

For "simple" conversions, we can take care of this by adding a variant of GAP.@install, say GAP.@install_rec (just a provisional name; we could also rename the current one to GAP.@install_leaf/GAP.@install_no_recursion/GAP.@install_basic/... and call the new one GAP.@install)

The new one would roughly convert

GAP.@install_rec GapObj(obj::MyType) = foobar(obj)

to something like this

function julia_to_gap_internal(obj::T, recursion_dict::Nothing, recursion::Bool) return foobar(obj) end function julia_to_gap_internal(obj::T, recursion_dict::RecDict, recursion::Bool) @assert recursion return get!(recursion_dict, obj) do return foobar(obj) end::Obj # TODO: is that right? end

Note that in general we can have recursion == true && recursion_dict === nothing; but it is up to any containers to allocate a recursion_dict if necessary. We don't do it here, meaning that julia_to_gap(123 ; recursive=true) just won't allocate a recursion dictionary, which is fine.

Regarding the type assertion on the get! return value: I think it must always be a GAP.Obj here? Perhaps we could even tighten it to GAP if we agree to just not store Int, Bool, and FFE values in recursion_dict (but then the conversion of BigInt needs some special casing perhaps... unless we decide to just never cache that one, then this problem essentially goes away, I think)

fingolfin · 2024-09-04T13:43:42Z

src/julia_to_gap.jl

    len = length(obj)
    ret_val = NewPlist(len)
    if recursive
+        if recursion_dict === nothing
+          recursion_dict = RecDict()


Another optimization note for future iterations on this: if we are given e.g. a Vector{Int16} and asked to recursively convert it to a GapObj, then we really don't need the recursion_dict, as there is no point storing the Int16 values in it.

To get rid of such things, my "dual" code in PR #777 does this:

if rec_dict === nothing && _needs_conversion_tracking(T) rec_dict = RecDict() end

where _needs_conversion_tracking(T) returns true if T is a type where the conversion results should be tracked.

Such an approach interacts nicely with my suggestion to "split" GAP.@install:

GAP.@install (or whatever it is called) could also produce a _needs_conversion_tracking method returning false

GAP.@install_rec (or whatever it is called) could also produce a _needs_conversion_tracking method returning true

Note: I guess we should have two different _needs_conversion_tracking functions, with different names, for the two direction: to and from Julia. Perhaps they'll end up identical but this is not a priori clear to me? Anyway there would be no harm in having two for the moment.

ThomasBreuer · 2024-09-06T13:45:11Z

The current test failures are due to crashes (Julia 1.11, Julia nightly) and due to a problem with @macroexpand (Julia 1.6).

As for the latter, one really gets a LoadError in Julia 1.6 for both the erroneous macro call and the corresponding @macroexpand call, whereas one gets no LoadError in Julia 1.8 for the @macroexpand call.

lgoettgens · 2024-09-06T14:41:48Z

For the 1.6 failure: The place in AbstractAlgebra where the macroexpand idea came from limits to some julia versions. See https://github.com/Nemocas/AbstractAlgebra.jl/blob/025eabc50948f7db78bd5561acfb73bd5b340cc8/test/Attributes-test.jl#L228.

ThomasBreuer · 2024-09-06T14:53:10Z

@lgoettgens Thanks.
Yes, I will do the same as AbstractAlgebra does.

ThomasBreuer · 2024-09-13T12:43:47Z

@fingolfin I think now it remains to decide where we really want recursion tracking.
Currently this is switched off for example for integers, via the various GAP.@install GapObj methods. In particular the additional GapObj_internal(x::Integer, ...) method does not strike.

fingolfin · 2024-09-12T13:13:40Z

src/julia_to_gap.jl

+# Methods for those types that want `false` have to be installed;
+# the `GapObj` methods arising from `GAP.@install` calls are unary,
+# therefore the macro automatically installs such a
+# `_needs_tracking_julia_to_gap` method.


Suggested change

# Methods for those types that want `false` have to be installed;

# the `GapObj` methods arising from `GAP.@install` calls are unary,

# therefore the macro automatically installs such a

# `_needs_tracking_julia_to_gap` method.

# Methods for those types that want `false` have to be installed explicitly.

# The `GapObj` methods arising from `GAP.@install` don't handle recursion

# and the macro automatically installs such a `_needs_tracking_julia_to_gap`

# method.

Logically, GAP.@install could handle conversion tracking (and thus require _needs_tracking_julia_to_gap to return true), we just chose not to (and we still could allow for it by providing a second such macro or an optional argument to that macro).

fingolfin · 2024-09-16T10:16:18Z

src/julia_to_gap.jl

+function GapObj_internal(x::Integer, cache::GapCacheDict, ::Val{recursive}) where recursive
    # if it fits into a GAP immediate integer, convert x to Int64
    x in -1<<60:(1<<60-1) && return Int64(x)
    # for the general case, fall back to BigInt
-    return julia_to_gap(BigInt(x))
+    return GapObj_internal(BigInt(x), cache, Val(recursive))
 end


It is OK to do this, but since we don't track conversion for any concrete integer types, I think it would be fine just say

Suggested change

function GapObj_internal(x::Integer, cache::GapCacheDict, ::Val{recursive}) where recursive

# if it fits into a GAP immediate integer, convert x to Int64

x in -1<<60:(1<<60-1) && return Int64(x)

# for the general case, fall back to BigInt

return julia_to_gap(BigInt(x))

return GapObj_internal(BigInt(x), cache, Val(recursive))

end

@install GapObj(x::Integer) = x in -1<<60:(1<<60-1) ? Int64(x) : GapObj(BigInt(x))

fingolfin · 2024-09-16T12:32:17Z

src/julia_to_gap.jl

+
+    rec = recursive && _needs_tracking_julia_to_gap(T)
+    if rec && recursion_dict === nothing
+        recursion_dict = RecDict()


Suggested change

recursion_dict = RecDict()

recursion_dict = RecDict()

(Formatting still seems to mix 2-space and 3-space wide indents -- this line was 6 spaces intended, now it is 5?)

fingolfin · 2024-09-16T12:38:14Z

src/julia_to_gap.jl

-) where {S} where {T<:Union{Symbol,AbstractString}}
+    recursion_dict::GapCacheDict,
+    ::Val{recursive},
+) where {S} where {T<:Union{Symbol,AbstractString}} where {recursive}


Suggested change

) where {S} where {T<:Union{Symbol,AbstractString}} where {recursive}

) where {S, T<:Union{Symbol,AbstractString}, recursive}

src/julia_to_gap.jl

If GAP.jl is loaded by another package then `GapObj` need not be available in the `Main` module.

- add `_needs_tracking_julia_to_gap(T)`, with default value `true` - change `GAP.@install` to install a `_needs_tracking_julia_to_gap` method - change the tracking logic: - Do not *create* a dictionary for tracking identical objects only if `_needs_tracking_julia_to_gap` for the subobject type returns `false`. - Let each object decide whether it is searched in the dictionary (in the beginning of its conversion method) or whether it gets added to the dictionary (as soon as the return value is available). For the current conversion methods for integers, rationals, etc., this means that these objects are *not* added to the dictionary, that is, identical large Julia integers are converted to nonidentical large integers in GAP; if we want to change this, we have to change their `GapObj` methods. - add a method for converting `Set{T}`: This had been added to Oscar.jl but should better be in GAP.jl. (There are tests for the return value whether the result is regarded as a set by GAP; perhaps we should restrict the allowed values of `T`.) - changed `GAP.Wrappers.Add(x::GapObj, y::GapObj, z::Int)` to `GAP.Wrappers.Add(x::GapObj, y::Any, z::Int)`, in order to avoid the automatic conversion `GapObj(y)` (Once the relevant code in Oscar.jl is changed to use `GAP.@install`, the current changes in GAP.jl do not need further changes in Oscar.jl.)

and make the assumption that `recursion_dict !== nothing` implies `recursive` explicit

The `_needs_tracking_julia_to_gap` method must be installed not only for the given type but also for its subtypes.

and add a test

- add helper functions for the recursive conversions (two are needed because some method specific variables must be created between the two parts) - do not distinguish between `nothing` and `RecDict` in the subobject conversion calls

... since this function got removed

ThomasBreuer · 2024-09-23T13:57:57Z

The required test matching: [Oscar#master] - ubuntu-latest, julia ~1.10.0-0 does not want to start, close and reopen ...

benlorenz · 2024-09-23T14:44:09Z

The required test matching: [Oscar#master] - ubuntu-latest, julia ~1.10.0-0 does not want to start, close and reopen ...

If there is a corresponding branch in the Oscar repository (or your fork) then the OscarCI will prefer that branch and the job will contain the branch name in the label (to make it clear which branch is being tested):

OscarCI / matching: [Oscar@ThomasBreuer#TB_julia_to_gap] - ubuntu-latest, julia ~1.10.0-0

Unfortunately there is no way to make the branch rules take that into account, they also just look at the label.

ThomasBreuer · 2024-09-23T15:03:50Z

@benlorenz thanks for your comment.

I had thought it would be a good idea to have corresponding branches with the same name in GAP.jl and Oscar.jl,
but apparently this only causes problems.
Should I just select "Merge without waiting for requirements to be met"?

And for the next breaking change in GAP.jl, should I better choose a different name for the corresponding branch in Oscar.jl?

benlorenz · 2024-09-23T15:36:00Z

It is meant as a feature, to be able to test whether the new code here would work together with the code in a corresponding branch for Oscar. That job is still running but it looks promising.

Since this is marked breaking (i.e. released as GAP.jl 0.12.x ?) it should not really matter whether it works with Oscar master? And in this case can be merged bypassing branch protections, maybe together with a version bump?

fingolfin · 2024-09-23T18:45:34Z

100% agree with @benlorenz

fingolfin · 2024-09-23T20:10:38Z

Weird failure in the OscarCI test on Julia 1.10 (in Aqua tests?). What is this dependency "jl_Te1ctwpcZO [9e52b259-76e2-4c08-b46d-33fd3af550ab]" ? Memory corruption?

GC: pause 1103.08ms. collected 0.010208MB. incr 
Test Summary:                         | Pass  Broken  Total  Time
canonical maps and garbage collection |    9       1     10  7.9s
Test Summary: |Time
issue 3107    | None  1.8s
-> Testing test/Modules/UngradedModules.jl took: runtime 12.538 seconds + compilation 49.101 seconds + recompilation 0.0 seconds, 4.491 GiB
Starting tests for /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/test/Aqua.jl
[ Info: Compiling JuliaInterface ...

GC: pause 198.71ms. collected 517.464280MB. incr 
ERROR: The following 2 direct dependencies failed to precompile:

jl_Te1ctwpcZO [9e52b259-76e2-4c08-b46d-33fd3af550ab]

Failed to precompile jl_Te1ctwpcZO [9e52b259-76e2-4c08-b46d-33fd3af550ab] to "/home/runner/.julia/compiled/v1.10/jl_Te1ctwpcZO/jl_KlMALp".
#I  The Julia package 'Downloads' cannot be loaded.
ERROR: LoadError: UndefVarError: `@install` not defined
Stacktrace:
 [1] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [2] include(x::String)
   @ Oscar ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:20
 [3] top-level scope
   @ ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/Groups.jl:1
 [4] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [5] include(x::String)
   @ Oscar ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:20
 [6] top-level scope
   @ ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:256
 [7] include
   @ ./Base.jl:495 [inlined]
 [8] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::String)
   @ Base ./loading.jl:2222
 [9] top-level scope
   @ stdin:3
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/types.jl:17
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/types.jl:17
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/Groups.jl:1
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:1
in expression starting at stdin:3
ERROR: LoadError: Failed to precompile Oscar [f1435218-dba5-11e9-1e4d-f1a5fab5fc13] to "/home/runner/.julia/compiled/v1.10/Oscar/jl_3nxY3m".
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
...

In a previous run the error was a bit different:

GC: pause 1146.51ms. collected 0.010208MB. incr 
Test Summary:                         | Pass  Broken  Total  Time
canonical maps and garbage collection |    9       1     10  8.0s
Test Summary: |Time
issue 3107    | None  1.8s
-> Testing test/Modules/UngradedModules.jl took: runtime 14.151 seconds + compilation 49.45 seconds + recompilation 0.0 seconds, 4.502 GiB
Starting tests for /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/test/Aqua.jl
[ Info: Compiling JuliaInterface ...

GC: pause 169.75ms. collected 516.809388MB. incr 
ERROR: The following 2 direct dependencies failed to precompile:

Oscar [f1435218-dba5-11e9-1e4d-f1a5fab5fc13]

Failed to precompile Oscar [f1435218-dba5-11e9-1e4d-f1a5fab5fc13] to "/home/runner/.julia/compiled/v1.10/Oscar/jl_LLtmII".
#I  The Julia package 'Downloads' cannot be loaded.
ERROR: LoadError: UndefVarError: `@install` not defined
Stacktrace:
 [1] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [2] include(x::String)
   @ Oscar ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:20
 [3] top-level scope
   @ ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/Groups.jl:1
 [4] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [5] include(x::String)
   @ Oscar ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:20
 [6] top-level scope
   @ ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:256
 [7] include
   @ ./Base.jl:495 [inlined]
 [8] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:2222
 [9] top-level scope
   @ stdin:3
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/types.jl:17
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/types.jl:17
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/Groups.jl:1
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:1
in expression starting at stdin:3
jl_W5mlE9sv4h [30b189bf-1215-468d-8b2f-55ea7a2af89b]

Failed to precompile jl_W5mlE9sv4h [30b189bf-1215-468d-8b2f-55ea7a2af89b] to "/home/runner/.julia/compiled/v1.10/jl_W5mlE9sv4h/jl_XxEYcw".
#I  The Julia package 'Downloads' cannot be loaded.
ERROR: LoadError: UndefVarError: `@install` not defined
Stacktrace:
 [1] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [2] include(x::String)
   @ Oscar ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:20
 [3] top-level scope
   @ ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/Groups.jl:1
 [4] include(mod::Module, _path::String)
   @ Base ./Base.jl:495
 [5] include(x::String)
   @ Oscar ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:20
 [6] top-level scope
   @ ~/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:256
 [7] include
   @ ./Base.jl:495 [inlined]
 [8] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::String)
   @ Base ./loading.jl:2222
 [9] top-level scope
   @ stdin:3
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/types.jl:17
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/types.jl:17
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Groups/Groups.jl:1
in expression starting at /home/runner/work/GAP.jl/GAP.jl/oscar-dev/Oscar/src/Oscar.jl:1
in expression starting at stdin:3
ERROR: LoadError: Failed to precompile Oscar [f1435218-dba5-11e9-1e4d-f1a5fab5fc13] to "/home/runner/.julia/compiled/v1.10/Oscar/jl_7vg26X".

benlorenz · 2024-09-23T20:24:02Z

Weird failure in the OscarCI test on Julia 1.10 (in Aqua tests?). What is this dependency "jl_Te1ctwpcZO [9e52b259-76e2-4c08-b46d-33fd3af550ab]" ? Memory corruption?

That is a temporary environment for the Aqua tests and there is an issue about Aqua not respecting the current environment (JuliaTesting/Aqua.jl#292) which might cause this test-environment to use a different GAP version...

ThomasBreuer · 2024-09-23T20:31:23Z

which might cause this test-environment to use a different GAP version...

This would explain the error message

ERROR: LoadError: UndefVarError: @install not defined

The macro GAP.@install gets introduced by this pull request, it is not known if any older GAP.jl is used.

fingolfin · 2024-09-24T06:25:47Z

Then let's move forward with this and make a release today

ThomasBreuer · 2024-09-24T08:21:09Z

Thanks for finally merging this pull request.

ThomasBreuer added kind: enhancement New feature or request topic: conversion Issue related to conversion labels Aug 30, 2024

ThomasBreuer requested a review from fingolfin August 30, 2024 09:23