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

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

Open
PallHaraldsson opened this issue Dec 11, 2024 · 8 comments

Comments

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Dec 11, 2024

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

JL_DLLEXPORT void jl_array_sizehint(jl_array_t *a, size_t sz)

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.

@adienes
Copy link
Contributor

adienes commented Dec 11, 2024

Image
probably not a good idea.

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Dec 11, 2024

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?

@oscardssmith
Copy link
Member

why are they ccalling rather than just using getindex?

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Dec 11, 2024

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 *array* -> *memory* in case people want to change that way, though not for that. It was a lot going through that huge PR, but a partial list 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:

     XX(jl_alloc_array_1d) \

-    XX(jl_alloc_array_2d) \
-    XX(jl_alloc_array_3d) \

at least 15 starting with jl_array dropped including the second one here, explaining your broken code:

-    XX(jl_arraylen) \
-    XX(jl_arrayref) \
- ...

-    XX(jl_array_ptr_copy) \
+    XX(jl_genericmemory_owner) \

-    XX(jl_new_array) \
 
-    XX(jl_ptrarrayref) \

-    XX(jl_reshape_array) \

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...:

ERROR: Unsatisfiable requirements detected for package CUDA [052768ef]:

I'm running test AutoGrad as we speak. It show me Warning I've not seen before, otherwise unrelated:

┌ Warning: Circular dependency detected. Precompilation will be skipped for:
│ Base.PkgId(Base.UUID("1285c0f1-ff9f-5867-b66e-0f359bcf09ba"), "SpecialFunctionsExt")
│ Base.PkgId(Base.UUID("63c18a36-062a-441e-b654-da1e3ab1ce7c"), "KernelAbstractions")

..

│ Base.PkgId(Base.UUID("f1d291b0-491e-4a28-83b9-f70985020b54"), "MLUtils")
└ @ Base.Precompilation precompilation.jl:511
Precompiling CUDA_Driver_jll...
✗ REPL
1 dependency successfully precompiled in 3 seconds. 27 already precompiled. 17 skipped due to circular dependency.

Update:

 Testing Running tests...

237.501511 seconds (100.92 M allocations: 4.972 GiB, 1.21% gc time, 99.71% compilation time: 1% of which was recompilation)
14.681524 seconds (14.41 M allocations: 743.913 MiB, 1.30% gc time, 99.40% compilation time: 9% of which was recompilation)
WARNING: Method definition loss(Any, Any, Any) in module Main at /home/pharaldsson/.julia/packages/AutoGrad/1QZxP/test/core.jl:12 overwritten at /home/pharaldsson/.julia/packages/AutoGrad/1QZxP/test/core.jl:17.
..
27.092265 seconds (12.97 M allocations: 669.565 MiB, 0.67% gc time, 99.84% compilation time)
Test Summary: | Pass Broken Total Time
AutoGrad | 573 16 589 9m49.7s
Testing AutoGrad tests passed

It seemingly can and should be optimized, and it's tests updated...

@adienes
Copy link
Contributor

adienes commented Dec 11, 2024

I'm searching in the docs. I don't think this has ever been anything remotely close to public api...

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Dec 11, 2024

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:

XX(jl_arraylen) \

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:
https://docs.julialang.org/en/v1/devdocs/builtins/

Core..memoryrefoffset

That's probably a typo .. -> .

FYI: We have a project like unoffical:
https://github.com/Suzhou-Tongyuan/TyJuliaCAPI.jl

Stable and generic C API for Julia

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.

@PallHaraldsson PallHaraldsson changed the title Reexport jl_arrayref, since it's still there, and broke at least one package Define stable C API and/or reexport jl_arrayref and jl_array_size since it's still there, and broke at least one package Dec 11, 2024
@Taaitaaiger
Copy link
Contributor

Taaitaaiger commented Dec 13, 2024

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 jl_value_t*, allocating it if necessary.

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Dec 14, 2024

I see it can't probably be reexported since now it's slightly different:

- JL_DLLEXPORT size_t jl_array_size(jl_value_t *a, int d)
+ JL_DLLEXPORT size_t jl_array_size(jl_array_t *a, int d)
  {
+     // n.b this functions only use was to violate the vector abstraction, so we have to continue to emulate that
+     if (d >= jl_array_ndims(a))

+        return a->ref.mem->length;
      return jl_array_dim(a, d);
  }

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

No branches or pull requests

4 participants