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

julia 1.11: add some workarounds for jl_array changes #137

Merged
merged 3 commits into from
Nov 16, 2023

Conversation

benlorenz
Copy link
Contributor

@benlorenz benlorenz commented Oct 28, 2023

for the changes in JuliaLang/julia#51319

cc: @fingolfin

Edit: The library seems to build but a test module fails with:

[ 87%] Linking CXX executable test_module
cd /home/runner/work/libcxxwrap-julia/libcxxwrap-julia/build/test && /usr/local/bin/cmake -E cmake_link_script CMakeFiles/test_module.dir/link.txt --verbose=1
/usr/bin/c++  -Wunused-parameter -Wextra -Wreorder -fPIC -g CMakeFiles/test_module.dir/test_module.cpp.o -o test_module   -L/opt/hostedtoolcache/julia/nightly/x64/lib  -L/opt/hostedtoolcache/julia/nightly/x64/lib/julia  -Wl,-rpath,/opt/hostedtoolcache/julia/nightly/x64/lib:/opt/hostedtoolcache/julia/nightly/x64/lib/julia:/home/runner/work/libcxxwrap-julia/libcxxwrap-julia/build/lib ../lib/libcxxwrap_julia.so.0.11.1 /opt/hostedtoolcache/julia/nightly/x64/lib/libjulia.so.1.11 
/usr/bin/ld: CMakeFiles/test_module.dir/test_module.cpp.o: in function `jl_datatype_layout':
/opt/hostedtoolcache/julia/nightly/x64/include/julia/julia.h:1310: undefined reference to `jl_unwrap_unionall'

Not yet sure what to make of this. This might be fixed by JuliaLang/julia#51916.

@PallHaraldsson
Copy link

I'm just curious did Julia break the C API? I mean it's not guaranteed stable yet I believe, except possibly some subset. But still practical breakage? Or is this about an added feature (that you could choose to now support)?

@benlorenz
Copy link
Contributor Author

I'm just curious did Julia break the C API? I mean it's not guaranteed stable yet I believe, except possibly some subset. But still practical breakage? Or is this about an added feature (that you could choose to now support)?

jl_arrayset seems gone (this was exported from libjulia.so) and jl_array_data needs a seconds (type-)argument now. These are the ones that I noticed for libcxxwrap.
So yes I think this is a changed C API, and if I remember correctly it was mentioned that this is expected to happen from time to time.

@fingolfin
Copy link
Contributor

Julia unfortunately has breaking C API changes all the time -- though at least these days they seem to avoid them in patch releases.

@PallHaraldsson
Copy link

PallHaraldsson commented Oct 28, 2023

It should at least be guaranteed to not happen in point releases, IMHO. Or people must then not upgrade (those who embed Julia, and well use C++ with...). There also doesn't seem like no reason for it (backporting needs to happen, but selectively, I doubt breaking the API needed for security, or even bug-fixes), Julia must be getting stable over time. So in fact also seems not needed for 1.x of changing x.

Adding to the API should be ok, I feel like at least a subset of the C API should be usable. Calling C code is ok, that has never been broken, so I'm unsure why does C++ actually need more, i.e. any of the C (embedding) API? Could this project restrict itself to some stable subset?

@fingolfin
Copy link
Contributor

We are talking here about fixes for the julia development version, not backports.

basically, for C API changes, minor releases 1.x are treated as semver major releases (so breaks happen) and patch releases as minor releases . Not great but I can see why to a degree. They are quite explicit about promising compatibility only at the language level, not for internal kernel APIs.

And nothing here is specific to C++

@barche
Copy link
Contributor

barche commented Oct 29, 2023

Regarding the jl_arrayset function I think it would be better to fall back to what is described in the embedding docs (link: https://docs.julialang.org/en/v1/manual/embedding/#Updating-fields-of-GC-managed-objects, I missed this when writing the array wrapper):

jl_array_t *some_array = ...; // e.g. a Vector{Any}
void **data = (void**)jl_array_data(some_array);
jl_value_t *some_value = ...;
data[0] = some_value;
jl_gc_wb(some_array, some_value);

I suppose that way we can also avoid the -ljulia-internal in the test?

@benlorenz
Copy link
Contributor Author

Regarding the jl_arrayset function I think it would be better to fall back to what is described in the embedding docs (link: docs.julialang.org/en/v1/manual/embedding/#Updating-fields-of-GC-managed-objects, I missed this when writing the array wrapper):

jl_array_t *some_array = ...; // e.g. a Vector{Any}
void **data = (void**)jl_array_data(some_array);
jl_value_t *some_value = ...;
data[0] = some_value;
jl_gc_wb(some_array, some_value);

This would be similar to the data()[i] = x in push_back for ArrayRef? Except that I am a bit unsure about the element type of that for Array, but void** would probably work. I can have a look later today.

Right now the only failure in the tests seems to be JuliaInterop/CxxWrap.jl#390 or am I missing something?

I suppose that way we can also avoid the -ljulia-internal in the test?

That workaround was only needed for avoiding the jl_unwrap_unionall issue (and worked only on linux), I removed the commit now.

@PallHaraldsson
Copy link

PallHaraldsson commented Oct 29, 2023

FYI, I just noticed with example "C++ Calling Julia", just about to be registered in case helpful to you:
https://github.com/Suzhou-Tongyuan/TyJuliaCAPI.jl

TyJuliaCAPI.jl provides a stable C API for Julia.
[..]
We have [several] projects implemented in C++/C#, and calling Julia from native code is a crucial requirement.
[..]
We, therefore, believe that the current quality of Julia's C API is insufficient to support product development and maintenance.

And we need a stable and generic Julia C API.

In the past, we have learned a useful technique from PythonCall.jl (referred to as GC Pooling), which PythonCall itself used to provide a set of interoperation mechanisms, surpassing similar projects in terms of stability and performance. Following this technique, we have implemented a stable and well-designed C API for Julia, which we call TyJuliaCAPI.

In TyJuliaCAPI, the lifecycle management is adopted in a manner similar to the Python Stable C API (the most widely applied and highly regarded C API design)

@fingolfin
Copy link
Contributor

Thanks for the pointer, but no, TyJuliaCAPI.jl is not helpful here. (and while I think it is a noble effort, some of the comments in the README strike me as rather naive -- holding up Python as a paragon for stable APIs, much less ABIs completely ignores history. The Python C API had over 30 years to evolve, and is notoriously is not ABI stable, and has other issues -- one of multiple reasons why e.g. HPy is a thing. We definitely want a more stable C API for Julia at some point, and there certainly are lessons to be learned from Python and other systems; we (OSCAR team) have suffered from and worked through Julia API/ABI changes constantly, so we appreciate the need. But at the same time there are good reasons why Julia is not yet there... In any case, any efforts to get such a stable API without involving the Julia core dev team seems doomed to fail ... though perhaps I just am just to old and cynical when it comes to this kind of stuff ...)

That said, this is all a red herring, it doesn't help us one bit with the issue at hand.

@benlorenz
Copy link
Contributor Author

I think the code here should be working, the tests still fail in the CI due to JuliaInterop/CxxWrap.jl#390, locally I get even more errors, see #391, because CxxWrap uses some internal string iteration methods that stopped working for the wide C++ strings.
Those errors don't appear in CI because it seems to use the testjll branch of CxxWrap.jl which is slightly outdated.

With the changes from this PR, JuliaInterop/CxxWrap.jl#390 applied and JuliaLang/julia#51671 temporarily reverted I got all tests to pass locally.

@benlorenz benlorenz marked this pull request as ready for review October 30, 2023 10:45
include/jlcxx/array.hpp Outdated Show resolved Hide resolved
@benlorenz benlorenz force-pushed the bl/jl111 branch 2 times, most recently from 1df29aa to 26c76fa Compare October 30, 2023 15:10
this avoids the arrayset and manually dealing with the write barrier
@benlorenz
Copy link
Contributor Author

benlorenz commented Oct 30, 2023

I changed the code to use jl_array_ptr_1d_push instead which does exist on all supported julia versions and simplifies the code quite a bit, this uses the (soon) recommended jl_array_ptr_set internally which deals with the write barrier.

@PallHaraldsson
Copy link

PallHaraldsson commented Oct 30, 2023

Python has (limited) stable API/ABI and maybe Julia should define similar JL_LIMITED_API:
https://docs.python.org/3/c-api/stable.html

There are two tiers of C API with different stability expectations:
[..] When Py_LIMITED_API is defined, only this subset is exposed from Python.h.
[..]
To enable this, Python provides a Stable ABI: a set of symbols that will remain compatible across Python 3.x versions. The Stable ABI contains symbols exposed in the Limited API, but also other ones – for example, functions necessary to support older versions of the Limited API.

I've not looked closely at TyJuliaCAPI.jl how they can promise a stable C API (and ABI?!) for Julia, presumably on top of its unstable C API, probably by locating the most practically stable subset, or thinking least likely to change, that could be defined stable (but hasn't...); for JL_LIMITED_API. I'm guessing that package is more of a promise to fix in the future if Julia changes... to at least localize in one place needed changes?

@barche
Copy link
Contributor

barche commented Nov 1, 2023

Tests here use the testjll branch of CxxWrap.jl, which I updated now to include the change from JuliaInterop/CxxWrap.jl#390, but because of JuliaInterop/CxxWrap.jl#391 nightly will probably still fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants