Add functions for setting default custom layers #608
Conversation
| export Layer, next, top_layer, insert | ||
| export Layer, next, top_layer, insert, insert_default!, remove_default! | ||
|
|
||
| const EXTRA_LAYERS = Set{Tuple{Union{UnionAll, Type{Union{}}}, UnionAll}}() |
There was a problem hiding this comment.
Hmmmm.....I don't quite understand the eltype of the Set here; why all the Tuple/Union/UnionAll dance?
There was a problem hiding this comment.
I was trying to keep all the type parameters as concrete as possible. The Set elements are Tuples with two elements: the first is the before layer, which can be either a Type{<:Layer} or Type{Union{}} (in the case that we insert a layer as the last layer), and the second is the custom layer, which is always a Type{<:Layer}. Since layer types are always parametric, a non-parameterized layer type is a UnionAll. Does that make sense? Maybe it's totally unnecessary to parametrize the Set like this.
|
When I saw BrokeRecord.jl I thought it was a good use case for a custom layer and was surprised that it instead used cassette. |
|
Yeah, it's not possible (AFAIK). If some transitive dependency calls |
|
Any more thoughts on this @quinnj? |
There was a problem hiding this comment.
Yeah, I don't love this approach, but I think it's fine w/ our current setup. I've started mulling over ideas for removing the strongly typed layer system for something a little simpler; like perhaps using an array of "layer functions" (which would work similarly to the approach in this PR), or something along those lines.
|
This feature is quite useful. One thing I do not understand is, why is a |
Some motivation for this: BrokenRecord.jl wants to add some behaviour to all calls to
HTTP.request, which sounds like a good case for a custom layer, but since it is not in control of whereHTTP.requestis called, it can't insert a modified stack into all those calls. Instead, it uses Cassette to achieve something similar, and it works, but it can be horribly slow. With the ability to add default layers, BrokenRecord would no longer need Cassette and would be 500000x faster and more reliable.Naming for these functions is open for discussion, I didn't think too hard about it.
I tried to keep the API similar to the existing
insertfunction.If something like this is actually wanted, I'll write some documentation for it.