-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Define stable C API and/or reexport jl_arrayref and jl_array_size since it's still there, and broke at least one package #56805
Comments
Where are you searching? I find 9 results now, two other packages including in Knet.jl (edited my top with the new link): https://juliahub.com/ui/Search?q=jl_arrayref&type=code apparently (one of its places, not all) CUDA related in src/cuarrays/cubytes.jl so did tests/PkgEval miss it, and/or is it not much used? Redundant now with Flux/Lux? |
why are they ccalling rather than just using |
Not sure, I'm just looking into what broke and where. Packages could be "fixed", but is it better to reexport? Do you know if it's enough, and the definition at the new place is equivalent (if not probably bad to keep the name... though might be the reason it was unexported...)? There might be way more needing fixing in the package[s], or not, I'm just starting investigating... I see lots of changes src/jl_exported_data.inc (only additions) src/jl_exported_funcs.inc (only deletions, except 1 addition I list): First one still available, unlike 2d, 3d variant:
At least PkgEval is off the hook, since AutoGrad passed tests and (otherwise the rest offtopic): I can't install Knet.jl actually on 1.11 nor 1.10, likely why it wasn't tested...:
I'm
Update:
It seemingly can and should be optimized, and it's tests updated... |
I'm searching in the docs. I don't think this has ever been anything remotely close to public api... |
Fair enough, still broke a package, and if not reexporting, then would like to know best way forward. @vtjnash, what exactly is the "Julia C API"? https://docs.julialang.org/en/v1/manual/embedding/ Is it nothing, expect what's documented (on that page)? It doesn't say stable or unstable or exported. So is there some list of approved (or unapproved, or risky)? You might think stc/jlapi.c is such a file (or julia.h, neither is jl_uv.c stable) but jl_yield (and with it jl_stdout_obj, previously breaking an R package, and jl_eof_error) were taken from it: f3d6904#diff-b07e60feeddb5c668754c38c886dd27ae077ff37ce2231e852491c9a122cfac6L445 I think the files controlling exporting with previously:
only means that you CAN/COULD call, not that you should. I tried to look up [un]stable in the docs and found "unstable" here, and:
That's probably a typo .. -> . FYI: We have a project like unoffical:
It calls at least jl_array_size and jl_array_rank and only the latter currently exported, likely the former was/is in 1.10, but no longer... so I suppose that package broken too. |
I agree an explicitly public C API would be very nice to have. While the problematic function was never mentioned in any API docs, it did serve a very useful purpose for external users of the C API: it doesn't require any knowledge about the layout of the array elements, but simply returns the element at a certain index of an array as a |
I see it can't probably be reexported since now it's slightly different:
|
Memory PR unexported jl_arrayref and broke Mousetrap.jl, which is unregistered, and in AutoGrad.jl and Knet.jl https://juliahub.com/ui/Search?q=jl_arrayref&type=code [EDIT: older wrong link or showing 2/3 more, probably false positives...:
https://juliahub.com/ui/Search?q=jl_array&type=packages ]
The search may be unreliable, it told me also used in ArrayAllez.jl but it at least passed tests for me. I also don't see where it's used in Mousetrap.jl or if more needs to be added/reexported:
https://www.reddit.com/r/Julia/comments/1h27zay/the_mousetrap_is_a_trap/
jl_arrayref is still defined and used by Julia itself (with a changed definition in a different file src/builtins.c now, array.c before, but I think the definitions may actually be equivalent, not sure). Not sure where @adienes is searing in answer below.
Adding to the top here, this was unexported, used by three packages, e.g. ArraysofArrays.jl (and Enzyme):
https://juliahub.com/ui/Search?type=code&q=jl_array_size
@thautwarm, @songjhaha TyJuliaCAPI.jl is likely broken because of this since it uses jl_array_size but it fails test (despite of this?! why PkgEval didn't catch it). Tests DO pass for ArraysofArrays but likely shouldn't have...
If you do decide to reexport, then while you're at it why not also merge my #56340, one other unexport that broke (R) package. I could add reexports to that PR if wanted.
jl_array_sizehint is in 1.10, no longer in 1.11 (since alpha1), still referenced (no actual use?) by Enzyme.jl:
julia/src/array.c
Line 1153 in 4976d05
Mooncake.jl (formerly Tapir.jl) seems to use (or do something with this unexported function in foreigncall.jl/reference in tests:
https://github.com/compintell/Mooncake.jl/blob/v0.4.60/test/rrules/foreigncall.jl#L15
I'm testing ⌅ [da2b9cff] + Mooncake v0.4.40 it just takes a long time, so will go to sleep.
The text was updated successfully, but these errors were encountered: