-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ensure inlined_ids
eltype of StdVector
is correct
#112
Conversation
Codecov Report
@@ Coverage Diff @@
## main #112 +/- ##
=======================================
Coverage 75.06% 75.06%
=======================================
Files 8 8
Lines 369 369
=======================================
Hits 277 277
Misses 92 92
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems liek the root cause here is that ObjectID
is an abstract type adn julia just punts and turns it into Any
? Or is ti CxxWrap's fault? Either way LGTM
buffer = ray_jll.LocalMemoryBuffer(serialized_arg, serialized_arg_size, true) | ||
metadata = ray_jll.NullPtr(ray_jll.Buffer) | ||
inlined_ids = collect(serializer.object_ids) | ||
inlined_refs = ray_jll.GetObjectRefs(worker, StdVector(inlined_ids)) | ||
inlined_ids = StdVector(collect(serializer.object_ids))::StdVector{ray_jll.ObjectID} | ||
inlined_refs = ray_jll.GetObjectRefs(worker, inlined_ids) | ||
ray_obj = ray_jll.RayObject(buffer, metadata, inlined_refs, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orthogonal to this PR but it seems liek these lines are identical to what's below and fiddly enough that they'd benefit from refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left these lines duplicated for now as the Python code has minor differences. If we adopt those differences in our Julia code they will differ.
This seems to be another CxxWrap corner case with empty vectors as having any elements in the julia> x = [FromRandom(ObjectID)]
1-element Vector{ObjectIDAllocated}:
ObjectIDAllocated(Ptr{Nothing} @0x0000600001cd8660)
julia> StdVector(x)
1-element CxxWrap.StdLib.StdVectorAllocated{ObjectID}:
ray_julia_jll.ObjectIDDereferenced(Ptr{Nothing} @0x0000600001e6c3c0)
julia> y = ObjectID[FromRandom(ObjectID)]
1-element Vector{ObjectID}:
ObjectIDAllocated(Ptr{Nothing} @0x0000600001cd8630)
julia> StdVector(y)
1-element CxxWrap.StdLib.StdVectorAllocated{ObjectID}:
ray_julia_jll.ObjectIDDereferenced(Ptr{Nothing} @0x0000600001e8a790) |
The
GetObjectRefs
function requires that aStdVector{ObjectID}
is passed in for theinlined_ids
argument. If we updateserializer.object_ids
to return aSet{ObjectID}
things break in an unexpected way asStdVector(collect(Set{ObjectID}())) isa StdVector{Any}
where asStdVector(collect(Set{ObjectIDAllocated}())) isa StdVector{ObjectID}
.Here's an overview which may help those trying to understand what's going on:
The type checks here should provide some protection about accidental changes.
Related to: JuliaInterop/CxxWrap.jl#367