-
Notifications
You must be signed in to change notification settings - Fork 19
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
get/set queries #23
base: master
Are you sure you want to change the base?
get/set queries #23
Conversation
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.
Wow, thanks a lot. I find the code super elegant, especially how you could "contain" the use of @generated
! Does composition work?
src/optics.jl
Outdated
""" | ||
mapfields(f, obj) | ||
|
||
Construct a copy of `obj`, with each property replaced by |
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.
Construct a copy of `obj`, with each property replaced by | |
Construct a copy of `obj`, with each field replaced by |
src/optics.jl
Outdated
@@ -185,7 +205,7 @@ $EXPERIMENTAL | |||
struct Elements end | |||
OpticStyle(::Type{<:Elements}) = ModifyBased() | |||
|
|||
function modify(f, obj, ::Elements) | |||
function modify(f, obj, ::Elements, object=List()) |
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.
There are other ideas what n argument modify could mean. I think defining it is not essential for this PR? So let's try to avoid it.
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.
Alternatively if you feel strongly about using modify
we can also discuss that.
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.
The reason for adding this argument is so we can use the same code to both Construct
new objects in and return tuples of fields, with Tuple
(for all) and Splat
(for some filtered subset). This is how Flatten.jl works, it's just impossible to read that that is whats happening - your structure is so much cleaner here.
We can also use a different method entirely to do this, this was just me trying to find the cleanest way of using everything already written here and build things so the other optics can use what Ive added.
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.
This method isn't working currently for that, it's just a dummy argument so it doesn't break.
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.
Ok sure, we can in the end decide whether to rename this. Competing ideas were to use extra args of modify for passing context around. Or have modify(f, obj, lens)
behave to map(f, itr)
as modify(f, objs..., lens)
behaves to map(f, itrs...)
.
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.
Better to keep the syntax open for map etc.
I've made them separate methods for now, we can work towards reducing the differences. Probably it should only affect the inner _modify
not modify
anyway.
Thanks! It was awful before I rewrote it with the structure here. I'm not sure how to do composition properly yet, it needs more thinking. That's the part I understand the least about Accessors.jl so far... but getting there. |
The iterator is broken too, I knew it couldn't be that easy lol... Fixed: but things are getting a bit less elegant - the iterator has to be passed around everywhere. Probably the end result of this is that things will mostly look clear like you say, but |
This is composing now. It hast to compose internally, as you want to compose with whatever fields are selected by Query, not with its return value. |
Can you give a few examples of what Query should do and how you think it should compose? For instance julia> using Accessors
julia> obj = (a=missing, b=1, c=(d=missing, e=(f=missing, g=2)))
(a = missing, b = 1, c = (d = missing, e = (f = missing, g = 2)))
julia> Query(ismissing)(obj)
(missing, 1, missing, missing, 2) I would have expected Also are querries supposed to be lenses? That is should @assert q(set(obj, q, val)) ≅ val
# You get what you set.
@assert set(obj, q, q(obj)) ≅ obj
# Setting what was already there changes nothing.
@assert set(set(obj, q, val1), q, val2) ≅ set(obj, q, val2)
# The last set wins. |
Yeah that's just a bug, the answer should be what you expect. I need to write a few more tests. It should follow the lens laws... notice modify just uses The few tests I had just randomly work as expected 😆 |
Ok this version should be working for lens laws, but with some problems with set for |
I just realized, that querries are not lenses in general. @assert q(set(obj, q, val)) ≅ val
# You get what you set.
@assert set(obj, q, q(obj)) ≅ obj
# Setting what was already there changes nothing.
@assert set(set(obj, q, val1), q, val2) ≅ set(obj, q, val2)
# The last set wins. I believe the first and second lens law should always hold, but the third may not: julia> using Accessors
julia> obj = (a=missing, b=1, c=(d=missing, e=(f=missing, g=2)))
(a = missing, b = 1, c = (d = missing, e = (f = missing, g = 2)))
julia> q = Query(ismissing);
julia> obj2 = set(obj, q, ["first", "wins", "here"])
julia> set(obj2, q, ["second", "should", "win"]) Not a huge deal, but we should be careful about using |
src/optics.jl
Outdated
|
||
$EXPERIMENTAL | ||
""" | ||
struct Properties end |
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.
Since we have ConstructionBase.getproperties
and ConstructionBase.setproperties
I wonder if we could base this on
set(obj, ::Properties, val) = setproperties(obj, val)
(::Properties)(obj) = getproperties(obj)
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.
Yeah that's probably better.
Yeah... the query would have to be updated for that to hold, e.g: julia> set(obj2, Query(x -> x isa String), ["second", "should", "win"])
(a = "second", b = 1, c = (d = "should", e = (f = "win", g = 2))) So it breaks 3. when the the values passed to Formally (for tuples at least) maybe its: islens = query(values) == values Also let me know what you think of the macros (theyre in the tests now too). They seem nice for simple use cases, just using the Query object is probably easier beyond something like this: julia> missings_obj = (a=missing, b=1, c=(d=missing, e=(f=missing, g=2)))
(a = missing, b = 1, c = (d = missing, e = (f = missing, g = 2)))
julia> @getall missings_obj isa Number
(1, 2)
julia> @setall ismissing(missings_obj) = (1.0, 2.0, 3.0)
(a = 1.0, b = 1, c = (d = 2.0, e = (f = 3.0, g = 2))) Edit: it feels like these examples should also work for an iterator like a single number or |
How do you feel about a dependency on Static.jl? Using |
|
Turns out Static.jl doesn't actually fix it, I'm just getting Revise.jl artifacts from this Julia bug: We'll have to settle for 20-100ns or so timings until that gets fixed. I can get everything under 2ns using Revise but it wont stick on restart, which has been driving me crazy. |
I think I have an alternative approach how this can be implemented. Let me first sketch a slow implementation, struct Query{S,D,O}
select::S
descend_condition::D
optic::O
end
function modify(f, obj, q)
modify(obj, q.optic) do o
if q.select_condition(o)
f(o)
elseif q.descent_condition(o)
modify(f, o, q)
else
o
end
end
end
pop(x) = first(x), Base.tail(x)
push(x, val) = (x..., val)
function setall(obj, q, vals)
modify(obj, q) do o
val, vals=pop(vals)
val
end
end
function getall(obj, q)
ret = ()
modify(obj,q) do o
push(ret, o)
o
end
return ret
end Now this is slow, because we change captured variables in a closure. How to fix this? function getall(obj, q)
initial_state = ()
_, final_state = modify_stateful((obj, initial_state), q) do o, state
new_state = push(state, o)
o, new_state
end
return final_state
end
"""
new_obj, new_state = modify_stateful(f, (obj,state), optic)
Here `f` has signature `f(::Value, ::State) -> Tuple{NewValue, NewState}`.
"""
function modify_stateful end
function modify_stateful(f, (obj, state1), optic::Properties)
# @generated function that produces the following code:
nt = getproperties(obj)
val1, state2 = modify_stateful(f, (nt.field1, state1), optic)
val2, state3 = modify_stateful(f, (nt.field2, state2), optic)
val3, state4 = modify_stateful(f, (nt.field3, state3), optic)
val4, state5 = modify_stateful(f, (nt.field4, state4), optic)
patch = (field1=val1, field2=val2...)
new_obj = setproperties(obj, patch)
new_state = state5
return (new_obj, new_state)
end
function modify_stateful(f, (obj, state), q::Query)
modify_stateful((obj,state), q.optic) do o,s
if q.select_condition(o)
f(o,s)
elseif q.descent_condition(o)
modify_stateful(f, (o,s), q)
else
o, s
end
end
end
function modify_stateful(f, (obj, state), ::ComposedOptic)
...
end
# all currently existing modify methods in Accessors.jl can be rewritten into `modify_stateful` in a straight forward fashion.
# Like with modify we want to use trait based dispatch `_modify_stateful(f, obj_state, optic, ::OpticStyle)`.
# `modify` may in the end be reimplemented in terms of `modify_stateful` if the compiler likes it. What do you think? |
Wow that looks like a flexible generalisation, lets use it. Thanks for putting the time in to rethink this. |
I wonder if we can capture generator syntax here to make the macros a bit more flexible. @getall (x for x in obj if select(x))
@getall Float64[x for x in obj if select(x)] # collects into a vector of Float64
@modify CuArray(x) for x in obj if x isa AbstractArray # move everything to GPU
@getall (x for x in obj |> Elements()) # customize the optic to flatten a nested tuple |
This is nice syntax: @getall Float64[x for x in obj if select(x)] # collects into a vector of Float64 You could also use lenses: @getall [x.a for x in obj if x isa NamedTuple]
@setall (x[2] for x in obj if x isa AbstractArray) = (1, 2 ,3) |
Not for this PR, but for a later stage I think we can connect this with Transducers. The idea is to turn function Transducers.foldl(rf, ReducibleCollection(obj, optic), init)
# TODO this is the wrong method to overload, but you get the idea
_, ret = modify_stateful((obj, init), optic) do o, state
new_state = rf(state, o)
o, new_state
end
return ret
end We would have: getall(obj, q) = foldl(push!!, ReducibleCollection(obj, q)) Benefits would be that we could directly apply reduction functions to queries and Transducers could take care of the output container (e.g. Tuple, Vector, lazy iterator...). |
Transducers interop sounds good for a future PR. I probably have to leave that to you as I haven't had time to get my head around transducers yet. |
Ah nice, this is a way to deal with simple "contextful" queries. |
The context that is harder to get is the parent object and field name (probably as a This was fairly straightforward but not finished: julia> missings_obj = (a=missing, b=1, c=(d=missing, e=(f=missing, g=2)))
(a = missing, b = 1, c = (d = missing, e = (f = missing, g = 2)))
julia> @getall (x.e for x in missings_obj if x isa NamedTuple)
((f = missing, g = 2),) |
I totally agree fieldnames are so useful. More general context is tough and IMO out of scope for this PR. But as you say the way to go piggy bag context into state. Probably via some |
I've implemented Unfortunately it's harder to optimise now, I've got some lingering type instabilities. Maybe you can see whats causing them. |
modify_stateful((obj, state), inner(q.optic)) do o, s | ||
if (q::Q).select_condition(o) | ||
(f::F)(o, s) | ||
elseif (q::Q).descent_condition(o) | ||
ds = descent_state(s) | ||
o, ns = modify_stateful(f::F, (o, ds), q::Q) | ||
o, merge_state(ds, ns) | ||
else | ||
o, s | ||
end |
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.
Thanks a lot for pushing this further. I also played around with implementing this, see this branch:
https://github.com/JuliaObjects/Accessors.jl/tree/modify_stateful
Like with your implementation I encountered inference issues. I do not fully understand them, but it seems the compiler dislikes this switch. To explore what's going on I replaced elseif (q::Q).descent_condition(o)
into elseif true
but the thing would still not infer. OTOH manually deleting the elseif
branch did help the compiler to infer.
Note I did this experimentation with my branch, which is somewhat different then yours, but I guess has the same underlying issues. Worth checking if things are better with the new ConstructionBase version and nightly julia.
If not I think it would be good to produce an MWE and report it upstream. I don't know if it is considered a bug, but maybe we'll get an explanation of why it is this way and how to work around it.
A quick and dirty workaround would be to introduce a more narrow query where you can only select and descent based on type. Then the if elseif ...
could be replaced by three methods.
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.
Ok interesting we both hit the same problem.
I think the differences are mostly just my code to handle context and optional construction in setall. But those aren't whats causing the instability...
It seems like the problem is calling modify_stateful
recursively? It still seems broken even without the elseif
, and the elseif
was fine in the previous version. I don't understand exactly what is different between these implementations and my previous one or Flatten.jl, but it could be the additional complexity of the return objects? Or there is a little more happening at runtime in modify_stateful
than I had in mapobject
or Flatten.jl, and the compiler is giving up on something?
I also saw no difference between ConstructionBase versions.
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 its the layers of anonymous functions. Flatten.jl has the shared code inside the generated functions, and simple separate generated functions for getall/setall (flatten/reconstruct).
I much prefer the clarity of the current solution here. But it seems like if we want this to compile away we cant use anonymous functions inside the recursion.
I can't get this version to compile away even if you don't return any of the updated objects - so Julia can't even tell that the anonymous function is not modifying external state somehow and runs it even if it's not used.
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.
It does not seem to be related to anonymous functions per see. I played with eliminating all anonymous functions.
I replaced code like
modify_stateful(...) do os
do_stuff(os, captured, variables)
end
with code like
struct DoStuff{C,V}
captured::C
variables::V
end
(f::DoStruff)(os) = do_stuff(os, f.captured, f.variables)
modify_stateful(DoStuff(captured, variables), ...)
It helped in very small examples, small examples still fail. Also, @inline
everything did not help. Also eliminating all @generated
functions by manually defining all ConstructionBase.f(::MyObject, ...)
did not help.
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.
Damn, that's exhaustive.
It feels like the compiler is using some heuristic to give up on solving the returned state of the recursion instead of just trying. A single non-recursive modify_stateful
compiles away completely, but it wont if called recursively.
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.
Ok, think i found the issue. If you define the select and descent conditions like this it's type stable:
select_condition(x::NamedTuple) = true
select_condition(x) = false
descent_condition(x::Tuple) = true
descent_condition(x) = false
So the compiler optimises these methods differently to methods that are passed in in any way. Even with the function in the type instead of fields, or just passing a Type to a 2 arg version of descend_condition
still doesn't work. It's just giving up and returning Any
. If you actually include the types to pass to 2 arg descend_condition(T, x)
in the code, it's type-stable.
I had assumed previously that using a type from a type parameters was practically the same as writing the type in the code manually, but it seems like it isn't.
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.
Awesome that you found the issue! This looks like a bug to me, can you maybe produce a MWE and report it upstream?
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.
Yeah I'm cutting it down to size, but its probably 1.6 inference problems like JuliaLang/julia#35800.
It's all type stable on 1.4 - that's why this feels weird for performance intuition. These things used to just work.
Actually no its still all #35800, even on 1.4!! if you compile if with the types hard coded once then it knows when you use revise with the types as parameters, and is type stable. Then it's not on restart.
@jw3126 sorry I haven't had time for this in a while, just revisiting now. How do you feel about these options:
|
IIRC queries of moderate complexity are not type stable, trivial queries work? |
Do you mean the query complexity or the complexity of the type being queried? If the former, having type stable |
@bgroenks96 your trick in Flatten.jl of mapping out the type beforehand might help here too? Idk. I was hoping the compiler might get better some julia version and handle this. It obviously can do it, because Revise.jl can "fix" the type stability. But if it will is another question. But agreed, type stable |
I can take a look when I have some time to see if it is applicable.... although I must say I am hesitant. That is some of the most dense, inelegant, and unreadable code I have written in years... |
Yeah, and we did put a lot of work into making this elegant. The bug that lets Revise.jl fix this also makes it hell to work on - you have to restart Julia to check your changes. |
|
Oh wow so its actually getting worse, not better |
Yeah, it got worse in 1.7. |
Ok, yes It seems like Flatten.jl also got worse with 1.7 - its a huge chunk of the TTFX for DynamicGrids.jl now. What surprised me recently is that type instability in these methods seems to have a large effect on compile time - if you look at So it precompiles a whole tree of abstract methods that will never be used, which of course fails to produce a known output type but costs like half a second to conclude that. You cant precompile this away either. |
So I guess I will continue sticking with 1.6 then... |
@rafaqz Perhaps we should consider cleaning up and generalizing the type tree concept in |
Can you tell if it is |
Pretty sure its |
Oof. Ok, then it might be the type tree actually causing it then. |
I wouldn't conclude that yet honestly, I'll have to look into it more. It's possible the problem is further out. But we should still think about the fact that the compiler will push abstract types through these methods to try and work out the result type. |
@bgroenks96 you might be right about generalising that package, but it is pretty annoying to do that work for the compiler. |
I can't be sure without explicitly testing this, but I think inference is failing because you are trying to operate on a complex type instead of the instance. I've encountered similar issues recently and you can get around it if you have a clear path through each struct's fields and are willing to do some work upfront with the instance instead of the type. |
Hmm maybe, but I'm not sure exactly what you mean. Do you have any specific lines/areas that could be changed? It looked to me like the compiler gives up on chasing function return types through nested recursive functions. Which also happens in Flatten.jl now. |
This functionality is implemented in AccessorsExtra, and works fine in my usage over recent months. julia> using AccessorsExtra
julia> obj = (a=missing, b=1, c=(d=missing, e=(f=missing, g=2)))
julia> o = RecursiveOfType(Number)
julia> xs = getall(obj, o)
(1, 2)
julia> setall(obj, o, xs .+ 1)
(a = missing, b = 2, c = (d = missing, e = (f = missing, g = 3)))
julia> o = RecursiveOfType(Missing) ∘ @optic _.c
julia> getall(obj, o)
(missing, missing)
julia> setall(obj, o, (:x, :y))
(a = missing, b = 1, c = (d = :x, e = (f = :y, g = 2))) Everything is type-stable, little to no overhead, differentiates automatically, and composes with other optics (2nd example). |
Nice, looks similar to Flatten.jl. Honestly I doubt this will ever be possible without the generated functions. So Im not sure what we will be waiting on to put it here... |
It's not just
Interestingly, Accessors itself is basically free of hacks/julia internals for now, and it's probably best for such a foundational package to stay that way. At some point when the code matures, it can be upstreamed from AccessorsExtra to Accessors, but I'm not sure if that movement would bring any tangible benefits by itself now. The optic works and can already be used by anyone, and |
Aside from implementation challenges, I noticed there's also a question of semantics: how should recursion work exactly. Should Convert all namedtuples to dicts, no matter how nested they are: julia> modify(Dict ∘ pairs, (a=1, bs=((c=1, d="2"), (c=3, d="xxx"))), RecursiveOfType(NamedTuple))
Dict{Symbol, Any} with 2 entries:
:a => 1
:bs => (Dict{Symbol, Any}(:d=>"2", :c=>1), Dict{Symbol, Any}(:d=>"xxx", :c=>3)) but the same decision can lead to pretty confusing julia> modify(x -> 10x, (a=1+2im,), RecursiveOfType(Number))
(a = 100 + 200im,)
julia> getall((a=1+(2//3)im,), RecursiveOfType(Number))
(1, 1, 1//1, 2, 3, 2//3, 1//1 + 2//3*im) Do you have any suggestions of what's the better/more principled default, and how to make it convenient to switch between recurse/not recurse into |
That is a very good point. We should support both. Naming is tricky though. Somewhat similar concepts are breadth first vs depth first search or |
Yeah, pre/post order is another option indeed! Applicable only when it recurses from T into nested Ts. modify(f, x, RecursiveOfType(T, order=:post)) # postwalk
modify(f, x, RecursiveOfType(T, recurse_nested=false)) |
In a recent ...plus a few other neat functionslike "unrecurcising" a recursive optic for advanced usecases: julia> obj = (a=1, b=([2,3,4,5], "", 6+7im))
julia> ConcatOptics(obj, RecursiveOfType(Real))
(@optic _.a) ++ (@optic _.b[1][∗]) ++ (@optic _.b[3].re) ++ (@optic _.b[3].im) Actually, Happy New Year :) |
Happy new year! We could add the |
This PR adds get/set queries as discussed in #9, returning a
Tuple
of values from an object, or setting object values from aTuple
Currently you can select objects by a
select
function, and choose where to descend with adescend
function.For macros I was thinking something like this could be nice:
TODO:
Properties/Elements
distinction, and maybe addFields
? this is using fieldnames in a@generated
functionNotes:
it seems difficult to match the performance of a standalone recursive generated function. Might get there with some work. The performance is fine. There is one type stabiulity issue left checking values have been changed to avoid usingconstructorof
- typeof val/state pairs are needed to track if a change occurred. Otherewise its a lot faster now when the types are unstable.fieldnames
in@generated
with theFields
optic I added is 10-100x better thanProperties
. But it may need some additions to ConstructionBase.jl to be legitimate. Flatten.jl has always done this, but it predates ConstructionBase.jl, and it's a bit dodgy. We could addfieldconstructor
andpropertyconstructor
that both default toconstructorof
, in case they need to be different.