oscar-system/gap.jl

GAP packages for Julia integration

gap.jl's Issues

crash in getproperty for GAPFuncsType

Asking for GAP.GAPFuncs.TRANSDEGREES segfaults, because TRANSDEGREES is an immediate integer, but our code does

    variable = ccall(:GAP_ValueGlobalVariable,MPtr,(Ptr{UInt8},),name_string)

so it expects an MPtr. Return type should instead be a union of MPtr, GapFFE, Int64.

Even with that fixed, that code should still also check that what it got is a function.

Issues with Julia forking

Right now, JuliaInterface contains this hack in JuliaImportPackage:

        #Try to compile package by loading it into external Julia
        #Get Julia path
        julia_path := ConvertedFromJulia( JuliaEvalString( "Sys.BINDIR" ) );
        Exec( Concatenation( julia_path, "/../bin/julia -e \"", callstring,
                  "\"" ) );

The reason for this (as I just learned from @sebasguts ) is that otherwise, Julia tries to compile the given package itself, which requires a fork; but that fails with some error (apparently in libuv)

For now, this workaround is good enough, but it'd be useful to figure out what exactly the problem is, and how to fix it properly.

Julia side calling into GAP must intercept GAP errors, too

Just got this:

julia> l = LibGAP.to_gap([1,2,3])
GAP: [ 1, 2, 3 ]

julia> l[1]
1

julia> l[0]
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `[]' on 2 arguments

return;  # <- typed in by me

signal (11): Segmentation fault: 11

Julia GC sometimes crashes in GAP

There is a crash in the Julia-GC-integration in GAP, that sometimes occurs when running the GAP testinstall test suite several times in the row. @rbehrends and me reproduced it in the past, and could not track it down so far.

In addition, recently we have been seeing crashes in the JuliaInterface test suite (e.g. here) that look like this:

...
     252 ms (192 ms GC) and 124KB allocated for import.tst
testing: /home/travis/build/oscar-system/GAPJulia/gap/pkg/JuliaInterface/tst/u\
tils.tst
# line 2 of 27 (7%)fatal: error thrown and no exception handler available.
ReadOnlyMemoryError()
TryMarkRange at /home/travis/build/oscar-system/GAPJulia/gap/src/julia_gc.c:484
GapTaskScanner at /home/travis/build/oscar-system/GAPJulia/gap/src/julia_gc.c:534
gc_mark_loop at /buildworker/worker/package_linux64/build/src/gc.c:2310
_jl_gc_collect at /buildworker/worker/package_linux64/build/src/gc.c:2666
jl_gc_collect at /buildworker/worker/package_linux64/build/src/gc.c:2837
CollectBags at /home/travis/build/oscar-system/GAPJulia/gap/src/julia_gc.c:643
FuncGASMAN at /home/travis/build/oscar-system/GAPJulia/gap/src/gap.c:904
...

Note that ReadOnlyMemoryError is actually a misnomer; this exception is also thrown when trying to read from memory we don't have access to. So my guess is that GapTaskScanner is scanning too much. Perhaps jl_task_stack_buffer needs to be adjusted again on the Julia side.

Use GAP's Julia version for compilation and linking

Instead of prividing a flag for the Julia version to compile and link against, we should take the version GAP uses.

Automatically unwrap MPtr Julia "objects" ?

I wrote a Julia function which just returns its first argument, and put it into GAP variable myfunc. Now I get this:

gap> myfunc(fail, 2);
<Julia: Main.ForeignGAP.MPtr()>

It would be very convenient (and should be super cheap!) to instead return a this "MPtr" object as a regular GAP object. I.e., so that I'd get:

gap> myfunc(fail, 2);
fail

Revise `ImportJuliaModuleIntoGAP`

@sebasguts came up with the following plan: the global variable Julia will point to a custom GAP object, for which methods for \. and IsBound. (and possibly RecNames) are installed. When accessing Julia.foobar, it queries if Main.foobar exists; if not, it errors out, otherwise that object is returned.

On the next level, Julia module objects (i.e., jl_module_t) are wrapped into custom GAP objects (similar or even identical to the Julia object). Then for these the \. method of course performs the lookup relative to the jl_module_t (in the Julia object, this would then be jl_main_module).

Generate coverage data for Julia code on Travis (for use in Codecov)

JuliaExperimental: replace INTOBJ_INT by ObjInt_Int8

Using INTOBJ_INT is not safe for inputs that don't fit into a GAP immediate integer. The simple fix is to use ObjInt_Int8 instead. But I don't quite know how to test that code, so I'll leave it to somebody else to make that change.

Provide more "Julia" implementations of functionality inside the GAP kernel

There is a bunch of internal GAP functions (and also functions from GAP packages, mostly the IO package) which potentially clashes with Julia. For an optimal experience of using "GAP-on-Julia", we should address these.

Foremost, GAP contains code for dealing with subprocesses, based on forking. This does not work when running with Julia integration (this also affects the IO package, via IO_fork; the worker pool code IO provides; and other packages building atop of this, e.g. the GAP Jupyter interface).
While I don't see what to do about IO_fork, we certainly can do something about subprocess launching in GAP. Indeed, I suggest that we re-implement all of that in pure Julia code, and ultimately disable the relevant GAP C kernel code completely. (For initial tests, this is not necessary, but on the long term, this will avoid "accidental" use of that code). I made a separate issue for this, #241 241

Then there is input/output; we already have several issues dealing with how to capture Julia output in GAP and GAP output in Julia. This could be a lot simpler if we could teach the GAP kernel to simply do its IO via Julia routines.

There is probably more (e.g. interrupt handling), but the above already is a good start.

How to organize Julia code

I have been thinking lately about how to organize the Julia Interface code between LibGAP.jl and JuliaInterface. Currently, most of the code seems to be positioned arbitrary, and while JuliaInterface works without LibGAP.jl, the converse is false.

What would you think about JuliaInterface loading code from LibGAP.jl? I would then remove more Julia code from JuliaInterface and move it to LibGAP.jl. Or do you regard it bad practice if JuliaInterface loads code that is not in its package folder?

Show unified stack traces

Develop a way to show stack traces of both Julia and GAP function call stacks in the right order when an error occurs.

Add documentation

Of course this may need to wait a bit more till the dust has settled, but this project definitely needs a lot more documentation

• developer docs (explaining various design decisions, e.g. what to convert when; and how and where various things are implemented, such as e.g. function calls).

• user guide: how to do various things from the GAP side resp the Julia side, with lots and lots of examples

Some more specific points:

• Document GAP.GAPFuncs
• ...

Setup Travis (and Codecov?) for this package

libgap.run_it should error out if GAP is already loaded

Implement convenience functions in Julia and in GAP for doing many common basic operations

It would be very convenient if there were default Julia implementations for getindex, setindex!, getproperty, setproperty! that work on any GapObj (and raise an error if not applicable).

Here is some untested rough draft (without a working LibGAP.jl, it is a bit annoying to work on this)

# lists
getindex(x::GapObj, i::Int) = GAP.GAPFuncs.ELM_LIST(x, to_gap(i))
setindex!(x::GapObj, v, i::Int) = GAP.GAPFuncs.ASS_LIST(x, to_gap(i), to_gap(v))

# matrix
getindex(x::GapObj, i::Int, j::Int) = GAP.GAPFuncs.ELM_LIST(x, to_gap(i), to_gap(j))
setindex!(x::GapObj, v, i::Int, j::Int) = GAP.GAPFuncs.ASS_LIST(x, to_gap(i), to_gap(j), to_gap(v))

# records
RNamObj(f::Symbol) = GAP.GAPFuncs.RNamObj(to_gap(string(f)))
getproperty(x::GapObj, f::Symbol) = GAP.GAPFuncs.ELM_REC(x, RNamObj(f))
setproperty!(x::GapObj, f::Symbol, v) = GAP.GAPFuncs.ASS_REC(x, RNamObj(f), to_gap(v))

Benchmark and optimize calls from Julia to GAP functions

Now that calls from GAP to Julia have been improved a lot, the same should be done for the other way around.

Right now, GAP function calls from Julia are handled via this function:

function(func::GapFunc)(args...)
    arg_array = collect(args)
    result = ccall(Main.gap_call_gap_func,Any,
                        (MPtr,Any),func.ptr, arg_array )
    return result
end

And then call_gap_func is defined like this:

jl_value_t * call_gap_func(void * func, jl_value_t * arg_array)
{
    jl_array_t * array_ptr = (jl_array_t *)arg_array;
    size_t       len = jl_array_len(array_ptr);
    Obj          arg_list = NEW_PLIST(T_PLIST, len);
    SET_LEN_PLIST(arg_list, len);
    for (size_t i = 0; i < len; i++) {
        SET_ELM_PLIST(arg_list, i + 1, gap_julia(jl_arrayref(array_ptr, i)));
        CHANGED_BAG(arg_list);
    }
    Obj return_val = CallFuncList((Obj)(func), arg_list);
    if (return_val == NULL) {
        return jl_nothing;
    }
    return julia_gap(return_val);
}

Note that this always creates a plist, even if there is no argument, or only a few. Then the GAP kernel function CallFuncList in most cases (for <= 6 arguments) will extract the entries of this plist (and effectively discard the plist afterwards, in that it won't be referenced anymore afterwars), and then invoke optimized function dispatchers for 0 to 6 arguments.

My guess is that we can get a measurable speedup by providing multiple function call handlers on the Julia side, namely for 0 to 6 arguments, and the current one is retained for calls with more arguments. This then of course needs to be matched on the GAP side (in the JuliaInterface kernel extension) with (probably rather simple) call handlers.

However, I strongly suggest to create some benchmarks first, like I did for calls the other way around, to see whether this has the desired effect of improving performance (and reducing memory pressure, by allocating fewer temporary plists).

intended behavior of `ConvertedToJulia`?

Currently ConvertedToJulia seems to return its input when no corresponding Julia object exists.
For example, ConvertedToJulia( Group( () ) ) returns the given GAP group.

In JuliaInterface/src/JuliaInterface.c,
Func_ConvertedToJulia calls Func_ConvertedToJulia_internal
and returns Fail if the result is zero, but the result is never zero.

gap_julia should check if julia_obj is an MPtr

... if julia_obj is not an MPtr, it should take appropriate steps, e.g. raise an error; or return NULL and then let call_gap_func check for NULL, and raise an error / return a suitable "fail" indicator.

GAP.gap_to_julia(Array{Int64,1},1) leads to infinite recursion

julia> GAP.gap_to_julia(Array{Int64,1},1)
ERROR: StackOverflowError:
Stacktrace:
 [1] gap_to_julia(::Type, ::Int64) at /Users/mhorn/Projekte/GAP/gap.reimer/pkg/GAPJulia/LibGAP.jl/src/gap_to_julia.jl:100 (repeats 39969 times)
 [2] top-level scope at none:0

Add more and systematic tests for all features

My dream would be if TDD (test driven development) was used. In the meantime, it would be great to increase our coverage, see https://codecov.io/gh/oscar-system/GAP.jl. Looking at it, there is a lot of code that is never exercised.

Improve cache in getproperty for GAPFuncsType

That cache is problematic, because it will become out of date if the identifier is changed on the GAP side.

This problem could be avoided, while still retaining most of the performance gain (which in turn I believe mostly stems from avoiding to convert a jl_symbol into a GAP string object), if instead of caching the function pointer, we cached the GAP GVar (an plain UInt value). Then that GVar value can be used to efficiently retrieve the currently assigned value, if any.

This cache of GVar values then could also be used in other places that want to give access to gap global variables.

Rename GAP.GAPObj -> GAP.Obj; GAP.GapFFE -> GAP.FFE; ...

Make "official" API

Currently, JuliaInterface.h contains some functionality that is used in JuliaExperimental, and could potentially be interesting for other packages using JuliaInterface and providing kernel extensions. Suggestions

• Clean up JuliaInterface.h
• Fix an API of safe to use functions
• Provide an API version number

JuliaExperimental: incorrect use of AbsInt and GcdInt; create_rational is dangerous

JuliaExperimental calls AbsInt and GcdInt with rationals as arguments, which cannot work and will lead to crashes and/or corrupt data.

Note there is a kernel function AbsRat which can be used instead of AbsInt. This function is also available on the GAP level as ABS_RAT, so it could also be called as GAP.GAPFuncs.ABS_RAT. However, this may be slower compared to the ccall, see also issue #68 . Another thing one could try is to simply use ccall(:AbsRat, ..., i.e. without setting up a wrapper in gap_macros.c

For GcdInt, it is unclear to me what the intended meaning is when computing the gcd of two rationals. I'd say it is always 1 (and GAP agrees), except of course if both inputs are 0.

Lastly, create_rational is rather dangerous, as it allows creating denormalized rationals, such as 2/1, 8/4, 3/0 or 2/-1. GAP is not prepared for this -- it expects rationals to always have a denominator > 1. If that rules is violated, then e.g. the rational 2/1 in GAP is not equal to the integer 2.

I suggest to instead call the GAP kernel function QuoRat, which accepts both rationals and integers as arguments.

Lastly, with the current master branch, the wrapper functions MyFuncLT in the kernel are no longer necessary, as LT etc. all were converted from macros into functions.

GAP catches user interrupt via Ctrl-C

Import from https://github.com/oscar-system/LibGAP.jl/issues/2

@ThomasBreuer wrote:

When one loads libgap into julia then a double ''-C'' entered in a long julia computation
is apparently caught by GAP,
with the effect that julia is left, with the well-known message that is printed by GAP:
"gap: you hit '-C' twice in a second, goodbye."

Is this intended?

@wbhart replied:

We had the same problem when we used to call Pari from Nemo. We will have to work on handlers so that the two can cooperate.
…

@sebasguts replied:

I think this might need some modification in LibGAP. A library should never implement a signal handler.

Since GAP sets the signal handler at start-up (in sysfiles.c), and GAP is loaded after Julia, and programs can only have one handler per signal, the Julia handler certainly gets overwritten. The proper solution would be that LibGAP exposes an interrupt-function, which could be used by the Julia signal handler to interrupt the GAP computation. I will talk to Markus about this issue. Probably a first step is to disable the signal handler for LibGAP, and then think about implementing some interrupt function.

Avoid underscores `_` as prefix for kernel functions

To quote e.g. https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html

In addition to the names documented in this manual, reserved names include all external identifiers (global functions and variables) that begin with an underscore (‘_’) and all identifiers regardless of use that begin with either two underscores or an underscore followed by a capital letter are reserved names.

Implement conversion between GAP large integers and Julia BigInt

How can we add tests for the Julia side of `GAPJulia`?

I would like to add some tests to the Julia side of GAPJulia. The problem is that we need a path to an appropriate GAP executable to start GAP in Julia. So how can we add a portable test setup on the Julia side? Should we use environment variables?

JuliaImportPackage uses `Exec`, should use `Process` instead

Exec allows no error checking or input/output control.

Deal with Julia rationals 1//0 and -1//0

By chance I just learned that Julia allows rationals with denominator 0, which are then normalized to 1//0 and -1//0. For these, isinf returns true, and they seem to behave like infinities.

I propose to convert these to GAP's -infinity and +infinity.

Questions on technical aspects of writing the manual(s)

I am wondering how best to write documentation for GAPJulia. The natural move is to document GAP code via GAPDoc and/or AutoDoc; Julia code via their system; and C code... not so much (or maybe via GAPDoc).

And for a lot of stuff this may be fine; but then there is a lot of commonality, too: Like, right now I am working on some text which discusses data conversion, and that would be of interest to people on both sides, so ideally would be available from both manuals.

But how to handle that? Two ideas for that:

• pick one of the two (?) manuals, and place it there; add a link to that in the other manual
• find a way to write a text in a common format, then generate inputs for both documentation systems from that, so that the identical text can appear in both manuals

Build system: drop need for --with-julia, always use the Julia against which GAP is linked

I will work on this, by dropping autoconf+automake in favor of a gac based build system (as e.g. generated by the latest PackageMaker).

Document IncludeJuliaStartupFile resp. the custom support for `.julia/config/startup.jl`

Also, make sure this feature is tested on Travis.

Add a mechanism to easily "pin" references to Julia objects in our C code

One way would be to add them to the GAP module state, as those automatically get marked. Or we could add these to "our" Julia module. Or we could set a custom ExtraMarkFuncBags in GAP to a function marking the relevant variables.

Agree on kernel code formatting and add .clang-format file ?

Consider dropping T_JULIA_FUNC and instead using T_FUNCTION objects with custom handlers

Consider these timings, where myfunc is a simple Julia function that simply returns its first argument:

gap> ListX([1..10^4], [1..2], {i,j} -> i);; time;
14
gap> ListX([1..10^4], [1..2], myfunc);; time;
595

So Julia is not competitive at all for function calls.

I think this could be improved by using T_FUNCTION objects to wrap Julia functions; this avoids various slow paths in calling the current T_JULIA_FUNC objects.

To do this, you need to store the pointer to the julia function somewhere in the T_FUNCTION. I think FEXS_FUNC should be OK for that. Also set BODY_FUNC to zero, so that GAP considers the T_FUNCTION object as a kernel handler.

Then rewrite Func_JuliaCallFunc0Arg etc. into DoJuliaCallFunc0Arg handlers, to be hooked into the various HDLR_FUNC slots. I could work on this if you are interested, but I thought I should first find out if there was a reason you haven't used this approach so far.

Error messages are just printed, not given to GAP

Currently, the content of Julia error message is just printed to stderr, not returned to GAP.

Fix this by providing an IOBuffer for the showerror call, read out the string in this IOBuffer, and give it back to GAP.

What are the rules for conversion when calling from GAP to Julia, and vice versa? And why are they this way?

It is right now rather unclear to me when and why something is converted when calling from GAP to Julia, and the other way around, and what the reason behind these decisions are. I think these should be carefully documented, and perhaps revised.

I was rather surprised when I discovered that calling a Julia function, which goes through a GAP CallFuncList method, automatically calls ConvertedToJulia on all arguments; and that this function performs non-trivial conversions, e.g. from T_STRING to a Julia string. There seems to be now way to turn that off, which means that certain things would be impossible to implement efficiently.

I would suggest that there should be a low level calling interface in both direction which only performs the absolutely minimal, free or super-cheap conversions. Basically, it has to deal with T_INT and T_FFE separately going from GAP to Julia; everything else is represented by a MPtr. The T_FFE probably need a need custom type, which internally just is an UInt64 or so; and actually, there is some merit in not converting a T_INT into an Int64... there are pros and cons for this.

Going from Julia to GAP, MPtr should be unwrapped, see #27; and perhaps the wrapper for T_FFE should be unwrapped (but even that is not completely "obvious" to me as desirable). Everything else would just be wrapped on the GAP side.

On this, one can build higher level convenience interfaces which perform extra conversions, possibly even more than are perform right now.

JuliaInterface tests fail because `ImportJuliaModuleIntoGAP( "GAPUtils" );` fails

The tests use ImportJuliaModuleIntoGAP( "GAPUtils" ); which fails right now (@sebasguts tells me because GAPUtils now is Main.GAPUtils an this makes things complicated)

Merge LibGAP.jl into GAPJulia ?

... as discussed during GAP Days Fall 2018

NewPlist used incorrectly in `to_gap` method

The following code creates a plist of capacity length(v) but size 0 (!), then fills it, but never sets the length.

function to_gap(v :: Array{T, 1} where T) :: MPtr
    l = NewPlist(length(v))
    for i in 1:length(v)
        l[i] = v[i]
    end
    return l
end

Share conversion code between LibGAP.jl and JuliaInterface

Right now, many conversions are implemented twice: once in the GAP C kernel extension, see e.g. _ConvertedFromJulia_internal and _ConvertedToJulia_internal; and once in Julia, see from_gap and to_gap in LibGAP.jl/src/conversion.jl.

There are multiple problems with that:

• we have to duplicate a lot of work
• which conversions resp. types are supported can and will differ
• error checking differs
• one side may support other types than the other

Therefore, we should think about ways that allow us to implement all these conversions once for both sides. Whether this is done in C, GAP or Julia, or a mix of them does not matter that much to me, as long as it is efficient and available from both sides, and flexible enough for our purposes.

JuliaExperimental fails to load when Nemo not present

Currently, there is a using Nemo in utils.jl, which produced an Error if JuliaExperimental is loaded when using a Julia without Nemo.

Maybe we should split up the utils.jl file and load the Nemo specific functions only if Nemo can also be imported into GAP?

redirect Julia's stderr?

Currently Julia error messages are visible on the screen
but they do not appear in GAP's log files.
For example, the ArgumentError: ... line from the GAP session

gap> Julia.Base.\/\/( 0, 0 );
ArgumentError: invalid rational: zero(Int64)//zero(Int64)
Error, JuliaError
not in any function at *stdin*:15
type 'quit;' to quit to outer loop
brk>

is missing from the corresponding GAP log file.

Would it be possible to redirect Julia's stderr
such that GAP's stderr is used instead?

Add Julia structure for GAP finite field elements (immediate objects)

JuliaImportPackage uses `JULIA_HOME` which doesn't exist in Julia 1.0

How to deal with Julia's error messages in test files?

GAP's function Test treats GAP's error messages in a special way,
only the first line of the message is expected in the test file,
not the whole message that appears in a normal GAP session.

When a Julia error happens, Test seems to expect the whole error message.
This is problematic because the message contains file paths.

Is there a way to teach Test to be happy with the first line of the message?

Revisit GAP.GAPFuncs

Right now, all GAP library level functions are automatically exported to Julia as GAP.GAPFuncs when the JuliaInterface package is loaded.

There are some issues with that, though: AddGapJuliaFuncs is only called once at the start, so any GAP functions added later on are not exported to Julia. This can be solved by calling the (currently undocumented) function AddGapJuliaFuncs. But I note that this takes ~700 milliseconds on my laptop, which isn't so nice (also in general, as it delays start up).

Perhaps GAP.GAPFuncs could be implemented in a fashion similar to what we now do for the Julia object on the GAP side, i.e., GAP.GAPFuncs.foo is looked up dynamically.

Alternatively, having a GAP.WrapGAPFunc function (in addition to or instead of GAP.GAPFuncs) and then writing e.g. MyFunc = GAP.WrapGAPFunc("MyFunc") in .jl files doesn't sounds so bad to me either.

Clean up and merge gc_integration branch

... and remove traces of the pre-Julia-GC-integration times.