Use libjulia for Libtask #2087
Use libjulia for Libtask #2087
Conversation
L/Libtask/build_tarballs.jl
Outdated
| # see https://github.com/JuliaPackaging/BinaryBuilder.jl/issues/336 | ||
| # ENV["CI_COMMIT_TAG"] = ENV["TRAVIS_TAG"] = "v" * string(version) | ||
| name = "Libtask" | ||
| version = VersionNumber("0.4.0-julia.$(julia_version.major).$(julia_version.minor)") |
There was a problem hiding this comment.
Unfortunately this trick doesn't work, pre-release versions like this are apparently rejected by the registry (see discussion on #1948).
There was a problem hiding this comment.
Ah I knew there would be some problem, I was just too lazy to read through the whole discussion 😄
There was a problem hiding this comment.
So the workaround is basically to manually map different versions of Libtask to different Julia versions?
There was a problem hiding this comment.
Yeah, I think so. As I understand it, the long-term plan for JLLs is to decouple the JLL version from the upstream software version anyway (as a lot of software has versioning schemes which are difficult to accurately squeeze into semver)
| jl_ptls_t ptls = jl_get_ptls_states(); | ||
| return (jl_task_t*)ptls->current_task; |
There was a problem hiding this comment.
So I don't really understand JuliaLang/julia#32812; so the following may be stupid, please forgive me: I am guessing that for some reason there is a problem with directly using ccall on jl_get_current_task, which is why you re-implement it here. Not sure what the problem is, though: Is it an inefficiency? Or wrong behaviour? Or what? (It might be cool to document this here in a quick comment?)
At the same time, this specific function is a source of ABI incompatibility. If you instead did the following, this function would work fine with all Julia versions from 1.3-1.6, regardless of which Julia version was used to produce the binary:
| jl_ptls_t ptls = jl_get_ptls_states(); | |
| return (jl_task_t*)ptls->current_task; | |
| extern JL_DLLEXPORT jl_value_t *jl_get_current_task(void); | |
| return jl_get_current_task(); |
Of course if an inefficiency is the reason for this code, then my suggestion makes no sense.
There was a problem hiding this comment.
Looking further, you dig far more into innards of the TLS and tasks anyway, so I guess this question is moot :-). Except that I still feel a comment explaining its existence beyond just pointing at an issue might be helpful, but I am just watching from the sidelines, so don't hesitate to ignore me on this :-)
There was a problem hiding this comment.
I just copied the file from Libtask.jl without looking at the concrete implementation 😄
It seems that the linked PR caused hangs on Julia 1.5: TuringLang/Libtask.jl#59
I agree that the comment could/should be improved. I guess @KDr2 might be able to explain the details here?
There was a problem hiding this comment.
Sorry, I didn't dive deeply enough into Julia's internal when I wrote this. At that time I found that calling to jl_get_current_task using ccall didn't work anymore after I updated Julia, then I found the commit(JuliaLang/julia#32812) which caused the issue by using git bisect, the newly added optimization (specialized codegen) for jl_get_current_task made it problematic (crash the process while calling it using call). So I add this new function to keep the old implementation and fix the problem.
|
|
||
| #include "julia.h" | ||
|
|
||
| // Workaround for JuliaLang/julia#32812 |
There was a problem hiding this comment.
OK, so I read this as saying: "JuliaLang/julia#32812 is fixing something we need but it is not available in all Julia versions so we work around this"; but you are really saying "JuliaLang/julia#32812 introduced a regression that we are working around". But where is the Julia issue which reports the regression and discusses solutions?
|
|
||
| # see https://github.com/JuliaPackaging/BinaryBuilder.jl/issues/336 | ||
| # ENV["CI_COMMIT_TAG"] = ENV["TRAVIS_TAG"] = "v" * string(version) | ||
| name = "Libtask" |
There was a problem hiding this comment.
BTW I don't see a good reason for renaming this to Libtask_julia or so -- why should it matter that this links against libjulia_jll? We don't generally mention other dependencies in the name, do we?
There was a problem hiding this comment.
I got the idea from the name of the library libcxxwrap_julia but the main argument would be that, contrary to what the name suggests, this is not a build file for a binary package of the C library Libtask but rather an implementation of task copying for Julia (which is mainly/only used by particle filter implementations in Turing).
|
I copied more flags from Libtask.jl - not sure if they are needed and/or useful here. |
|
Do we want to drop the support for Julia 1.3 and earlier version? In the C source, there are a few conditional compilations that differ between Julia <=1.1/1.2 and its later versions, so If we still want to support Julia <= 1.2, only compiling one shared object may not work, otherwise, we can eliminate the conditional compilations in the C source and make libtask only support Julia >=1.3. |
|
Yes, good point. I think we should drop support for Julia < 1.3 (both in Libtask.jl, as suggested in TuringLang/Libtask.jl#76, and here) since the library can't built with libjulia and is not supported by Turing anymore (I'm not sure if any other packages depend on Libtask at the moment?). Users with older Julia versions can always use existing versions of Libtask.jl if needed. Maybe I misunderstood the last part of your comment but I just want to clarify that the intention of this PR is not to build one shared library for Julia >= 1.3 but one library for each Julia version >= 1.3. I'm not sure yet how this is achieved by the pipeline since |
Oh, I didn't realize this is achieved by this PR, I am also curious about how it is done, maybe by the I am also wondering how are the shared objects for each version of Julia named/placed in the JLL package... |
Initially, I had just copied the approach in |
|
Two points: JLLs only really work in Julia >= 1.3 (there are tricks around that, but I'll ignore that), so I'd suggest dropping support for 1.0 - 1.2 is fine And to get multiple versions for multiple Julia versions: In Julia >= 1.6, it will hopefully be possible by building a single JLL which contains separate binaries for Julia 1.6, 1.7, etc. (using the So the workaround there then is this: you submit this package several times, once for each Julia version to be supported: first this PR adds a JLL for Julia 1.3 (the julia_compat entry ensures it can only be used there). Once it is merged, you submit another PR which modifies the This is admittedly tedious, but at least not super difficult. |
|
Can someone please summaries me where we are with this PR? |
|
The build errors on Windows are fixed and I tried to incorporate all comments and suggestions. The build script now includes all flags that are currently set in https://github.com/TuringLang/Libtask.jl/blob/v0.4.2/deps/Makefile and should compile the library in the same way as currently done but by using libjulia_jll instead. Hence support for Julia < 1.3 is dropped. The intended versioning scheme is:
This PR builds version 0.4.0 for Julia 1.3, the other versions require subsequent PRs with an update of Does this seem fine to you? @fingolfin and @KDr2, have you additional comments or suggestions? |
|
It's fine to me. |
|
Can this be merged? |
In this PR I tried to use
libjuliafor building Libtask instead of manually downloading different Julia versions.To a large extent I just copied #1948, which got me thinking if maybe the name
Libtask_juliaorlibtask_juliawould indicate more clearly that the library depends on the Julia version. I assume the nameLibtaskwas chosen initially since so far the source code of the library is contained in https://github.com/TuringLang/Libtask.jl. I copied it over to Yggdrasil to get rid of this circular dependency and address TuringLang/Libtask.jl#75 and TuringLang/Libtask.jl#76 (comment).