Allow ScopedValue defaults to be lazily computed #59372
Conversation
Keno
force-pushed
the
kf/lazyscopedvalue
branch
2 times, most recently
from
7d9026b to
b879797
Compare
|
Note to self, the reason this likely passed where #53462 did not is that we're not propagating JULIA_TEST_RECORD_PASSES down into the exec anymore. We should either restore the withenv or preferably explicitly pass it down in the exec. |
base/scopedvalues.jl
Outdated
| ScopedValue{T}(val) where T = new{T}(true, val) | ||
| ScopedValue(val::T) where T = new{T}(true, val) | ||
| end | ||
| const ScopedValue{T} = Union{LazyScopedValue{T, Returns{T}}, LazyScopedValue{T, typeof(no_default)}} |
There was a problem hiding this comment.
Isn't this considered breaking, since ScopedValue{Foo} is no longer concrete? Why not create a new abstract type and make ScopedValue subtype that, thereby making handling of LazyScopedValue opt-in?
There was a problem hiding this comment.
I don't know that is technically breaking - the interface is the same, but yes it is a bit overly clever. I somewhat liked the uniformity of only having one object and providing the existing behavior as an API, but we can switch to separate struts.
There was a problem hiding this comment.
One of the invariants in my head for scoped values is that they are "constant within the same context", with the current lazy version it is possible to observe multiple separate states within the same context, by virtue of the user doing something like ()->rand() as the get_default.
So I think we should do the struct split and make the lazy version use OncePerProcess explicity instead of just recommending it's use. I don't see a use for OncePerThread, and OncePerTask screams like we should have a TaskLocalValue.
|
Alright, worth the old college try. I'm ok with forcing OncePerProcess here, although I still want the constructor to take that object explicitly, so we can decide to expand in the future if we want to. |
Keno
force-pushed
the
kf/lazyscopedvalue
branch
from
b879797 to
86c2b15
Compare
|
Updated as discussed (Claude helped). |
Actually, there's no point in doing this, because we don't store those in results.json anyway. We could consider serializing the results from the _exec and storing them in the parent test set for this purpose, but that is a separate change (cc @IanButterworth on whether this is desirable). |
|
I don't recall if I was aware that the child processes weren't recording. I like the idea of optionally passing testsets up from the child processes, but yeah I think that can be a separate issue. |
There was a problem hiding this comment.
Looks good to me! Could you also add a few tests to https://github.com/JuliaLang/julia/blob/master/test/scopedvalues.jl
| LazyScopedValue requires Julia 1.13+. | ||
| """ | ||
| mutable struct LazyScopedValue{T} <: AbstractScopedValue{T} | ||
| const getdefault::OncePerProcess{T} |
There was a problem hiding this comment.
It it worth abstracting over what OncePer is used here (just use F <: Function)? We could potentially consider also using Returns{T}(default) and throw_key_error() = throw(KeyError(self)) to re-combine LazyScopedValue/ScopedValue, though I don't know if that that is important. There is not (and cannot be) a convert method for OncePerProcess{S} to OncePerProcess{T}, so it could be a bit tedious sometimes if users are trying to write the type parameters explicitly while also using OncePerProcess's auto-interferred parameter ability (though users really shouldn't be writing the LazyScopedValue apply_type explicitly)
There was a problem hiding this comment.
That's what this PR did originally, but @vchuravy didn't like it. Regardless, it is always possible to relax the implementation in that direction if it becomes apparent that it is useful, so this is the conservative version.
There was a problem hiding this comment.
Sure, makes sense. I think flexibility would make more sense with alternatives like OncePerDepot than the ones Valentin mentioned or OncePerPeriod (aka debounce or throttle). But as you say, this is conservatively what is actually needed now
Test has `Test.record_passes()` which gets `OncePerProcess` initialized to the value of `ENV["JULIA_TEST_RECORD_PASSES"]`. However, in another palce the test suite once to override this value for its scope, and does this by setting the environment variable, relying on OncePerProcess picking this up. However, this is not at all semantically guaranteed and of course fails if anybody happened to have a testset before (which is possible, because this is used by Base.runtests). It would be much better if we could use a ScopedValue for this like the other `Test` setting state (after #53462). However, there currently isn't any way to once-initialize the ScopedValue default. This PR aims to fix that by letting the scoped value default be an evaluated callback.
Keno
force-pushed
the
kf/lazyscopedvalue
branch
from
e56ac8a to
04cf53f
Compare
| implementation is available from the package ScopedValues.jl. | ||
| """ | ||
| mutable struct ScopedValue{T} | ||
| # NOTE this struct must be defined as mutable one since it's used as a key of |
There was a problem hiding this comment.
Should these commente be preserved? They seem like important invariants.
There was a problem hiding this comment.
I don't care much either way - and I probably would have kept them if not for the detour that deleted this, but it's also just how julia works, so I don't think they're really all that valuable.
There was a problem hiding this comment.
Adding the comment back seems reasonable because a mutable struct with all const fields seems like something that should be immutable apart from this inobvious reason.
There was a problem hiding this comment.
Added back (since you said you didn't mind either way @Keno)
IanButterworth
removed
the
merge me
IanButterworth
added
the
merge me
Co-authored-by: Sukera <[email protected]>
|
Should packages be using AbstractScopedValue (and/or LazyScopedValue)? I.e. should it be backported to 1.12 (even 1.10?), even just the abstract type. I'm not too up-to-speed on ScopedValues, but just looked into, and it seems at least one package should use AbstractScopedValue here: |
|
New feature are not backported. |
|
AbstractScopedValue is not a feature (or very borderline). I was thinking should most abstract types be in Base (at least for subtypes of Base)? Is it helpful to have it, even if LazyScopedValue isn't yet defined by a certain Julia version, to enable package system to use the abstract type, and help for future compatibility. Would it otherwise need to be in Compat.jl? |
7 participants
Test has
Test.record_passes()which getsOncePerProcessinitialized to the value ofENV["JULIA_TEST_RECORD_PASSES"]. However, in another palce the test suite once to override this value for its scope, and does this by setting the environment variable, relying on OncePerProcess picking this up. However, this is not at all semantically guaranteed and of course fails if anybody happened to have a testset before (which is possible, because this is used by Base.runtests). It would be much better if we could use a ScopedValue for this like the otherTestsetting state (after #53462). However, there currently isn't any way to once-initialize the ScopedValue default. This PR aims to fix that by letting the scoped value default be an evaluated callback. The originalScopedValuestruct is renamedLazyScopedValue, but theScopedValuename is retained with the same API as an alias to aLazyScopedValuethat just usesReturns(default)as its initializer. It might be a bit weird that the default could return a different value at every time, but I think a reasonable way to think about this is that the default chains to some dynamic state which is expressed in this callback.