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

Support deprecating checkpoint names #45

Merged
merged 4 commits into from
Aug 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "Checkpoints"
uuid = "b4a3413d-e481-5afc-88ff-bdfbd6a50dce"
authors = "Invenia Technical Computing Corporation"
version = "0.3.17"
version = "0.3.18"

[deps]
AWSS3 = "1c724243-ef5b-51ab-93f4-b0a88ac62a95"
Expand Down
50 changes: 43 additions & 7 deletions src/Checkpoints.jl
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ using Memento
using OrderedCollections

export checkpoint, with_checkpoint_tags # creating stuff
export enabled_checkpoints
export enabled_checkpoints, deprecated_checkpoints
# indexing stuff
export IndexEntry, index_checkpoint_files, index_files
export checkpoint_fullname, checkpoint_name, checkpoint_path, prefixes, tags
Expand All @@ -29,7 +29,7 @@ __init__() = Memento.register(LOGGER)

include("handler.jl")

const CHECKPOINTS = Dict{String, Union{Nothing, Handler}}()
const CHECKPOINTS = Dict{String, Union{Nothing, String, Handler}}()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this is using types to encode whether the checkpoint is deprecated or not. Thus it uses a String type to denote a deprecated status. Under the hood, it basically encodes a Pair that maps old -> new

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. We could encode this information in as Dict{String, AbstractHandler}() and have dedicated Disabled and Deprecated types. In the end, the string is just acting as a reference.

@contextvar CONTEXT_TAGS::Tuple{Vararg{Pair{Symbol, Any}}} = Tuple{}()

include("session.jl")
Expand Down Expand Up @@ -72,9 +72,19 @@ available() = collect(keys(CHECKPOINTS))
"""
enabled_checkpoints() -> Vector{String}

Returns a vector of all enabled ([`config`](@ref)ured) checkpoints.
Returns a vector of all enabled ([`config`](@ref)ured) and not [`deprecate`](@ref)d checkpoints.
Use [`deprecated_checkpoints`](@ref) to retrieve a mapping of old / deprecated checkpoints.
"""
enabled_checkpoints() = filter(k -> CHECKPOINTS[k] !== nothing, available())
enabled_checkpoints() = filter(k -> CHECKPOINTS[k] isa Handler, available())
morris25 marked this conversation as resolved.
Show resolved Hide resolved

"""
deprecated_checkpoints() -> Dict{String, String}

Returns a Dict mapping deprecated checkpoints to the corresponding new names.
"""
function deprecated_checkpoints()
return Dict{String, String}(filter(p -> last(p) isa String, CHECKPOINTS))
end

"""
checkpoint([prefix], name, data)
Expand Down Expand Up @@ -131,9 +141,7 @@ If the first argument is not a `Handler` then all `args` and `kwargs` are passed
"""
function config(handler::Handler, names::Vector{String})
for n in names
haskey(CHECKPOINTS, n) || warn(LOGGER, "$n is not a registered checkpoint")
debug(LOGGER, "Checkpoint $n set to use $(handler)")
CHECKPOINTS[n] = handler
_config(handler, n)
end
end

Expand All @@ -149,6 +157,20 @@ function config(prefix::Union{Module, String}, args...; kwargs...)
config(Handler(args...; kwargs...), prefix)
end

# To avoid collisions with `prefix` method above, which should probably use
# a regex / glob syntax
function _config(handler, name::String)
haskey(CHECKPOINTS, name) || warn(LOGGER, "$name is not a registered checkpoint")

# Warn about deprecated checkpoints and recurse if necessary
if CHECKPOINTS[name] isa String
Base.depwarn("$name has been deprecated to $(CHECKPOINTS[name])", :config)
return _config(handler, CHECKPOINTS[name])
else
debug(LOGGER, "Checkpoint $name set to use $(handler)")
return setindex!(CHECKPOINTS, handler, name)
end
end

"""
register([prefix], labels)
Expand All @@ -171,4 +193,18 @@ function register(prefix::Union{Module, String}, labels::Vector{String})
register(map(l -> join([prefix, l], "."), labels))
end


"""
deprecate([prefix], prev, curr)

Deprecate a checkpoint that has been renamed.
"""
function deprecate end

deprecate(prev, curr) = setindex!(CHECKPOINTS, curr, prev)

function deprecate(prefix::Union{Module, String}, prev, curr)
deprecate(join([prefix, prev], "."), join([prefix, curr], "."))
end

end # module
19 changes: 19 additions & 0 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,25 @@ Distributed.addprocs(5)

Checkpoints.config("c2", path)
@test enabled_checkpoints() == ["c1", "c2"]

# Manually disable the checkpoint again
Checkpoints.CHECKPOINTS["c1"] = nothing
Checkpoints.CHECKPOINTS["c2"] = nothing
end
end

@testset "deprecated" begin
mktempdir() do path
@test deprecated_checkpoints() == Dict(
"TestPkg.quux" => "TestPkg.qux_a",
"TestPkg.quuz" => "TestPkg.qux_b",
)

@test_deprecated Checkpoints.config("TestPkg.quux", path)
@test enabled_checkpoints() == ["TestPkg.qux_a"]

# Manually disable the checkpoint again
Checkpoints.CHECKPOINTS["TestPkg.qux_a"] = nothing
end
end

Expand Down
8 changes: 6 additions & 2 deletions test/testpkg.jl
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
module TestPkg

using Checkpoints: register, checkpoint, with_checkpoint_tags, Session
using Checkpoints: deprecate, register, checkpoint, with_checkpoint_tags, Session

# We aren't using `@__MODULE__` because that would return TestPkg on 0.6 and Main.TestPkg on 0.7
const MODULE = "TestPkg"

__init__() = register(MODULE, ["foo", "bar", "baz", "qux_a", "qux_b", "deprecated"])
function __init__()
register(MODULE, ["foo", "bar", "baz", "qux_a", "qux_b", "deprecated"])
deprecate(MODULE, "quux", "qux_a")
deprecate(MODULE, "quuz", "qux_b")
end

function foo(x::Matrix, y::Matrix)
# Save multiple variables to 1 foo.jlso file by passing in pairs of variables
Expand Down