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.

Sign up for GitHub

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

Jump to bottom

Add functions to check enabled checkpoints #26

Merged
merged 3 commits into from
May 27, 2021
Merged

Add functions to check enabled checkpoints #26

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e36385
Add functions to check enabled checkpoints
bencottier May 27, 2021
bd4a13e
Make is_enabled private, export enabled
bencottier May 27, 2021
9047404
Rename enabled to enabled_checkpoints
bencottier May 27, 2021
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
View file
Open in desktop
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.8"
version = "0.3.9"

[deps]
AWSS3 = "1c724243-ef5b-51ab-93f4-b0a88ac62a95"
Expand Down
16 changes: 16 additions & 0 deletions src/Checkpoints.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,22 @@ Returns a vector of all available (registered) checkpoints.
"""
available() = collect(keys(CHECKPOINTS))

"""
enabled() -> Vector{String}

Returns a vector of all enabled ([`config`](@ref)ured) checkpoints.
"""
enabled() = filter(is_enabled, available())

"""
is_enabled(name) -> Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_enabled(name) -> Bool
_isenabled(name) -> Bool

i.e. no _ to be consistent with julia convention, e.g. isa isqrt isone isodd isnan isinf isdir iszero isreal.

My preference is usually to keep the API as small as possible, because that reduces the probability of having to make a breaking change. So in this case I would just go with exporting enabled(). We can always export _isenabled later if there is a need.

p.s. we should export enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference is usually to keep the API as small as possible

That makes sense, I was thinking that we may as well make it "available" (not exported but not _'d either). But it's not needed yet.

I didn't export enabled by analogy to available not being exported. Does that change your view at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like config is not exported either (I think we should export it).

Let's export enabled, but not available (since there seems to be no use case for available outside of the package, but there is for enabled).

@oxinabox thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A reason I see for not exporting these functions is readability in user code. It's not obvious that config is about checkpoints if the call is e.g. config(["value1", "value2"], path). Even more so for enabled() on its own. If we exported it, I would prefer to call it enabled_checkpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mzgubic let me know if it looks good now

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM!

is_enabled(names...) -> Bool

Returns `true` if the checkpoint `name`(s) are [`config`](@ref)ured.
"""
is_enabled(name) = haskey(CHECKPOINTS, name) && CHECKPOINTS[name] !== nothing
is_enabled(names...) = all(is_enabled.(names))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is_enabled(names...) = all(is_enabled.(names))

Do we need that? Also, it is probably confusing to see is_enabled (i.e. singular) with several arguments.


"""
checkpoint([prefix], name, data)
checkpoint([prefix], name, data::Pair...)
Expand Down
18 changes: 18 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,24 @@ Distributed.addprocs(5)
@testset "Checkpoints" begin
@everywhere include("testpkg.jl")

@testset "enabled" begin
mktempdir() do path
Checkpoints.register(["c1", "c2"])
@test Checkpoints.enabled() == []

@test !Checkpoints.is_enabled("c1")
Checkpoints.config("c1", path)
@test Checkpoints.is_enabled("c1")
@test Checkpoints.enabled() == ["c1"]

@test !Checkpoints.is_enabled("c1", "c2")
Checkpoints.config("c2", path)
@test Checkpoints.is_enabled("c1", "c2")

@test Checkpoints.enabled() == ["c1", "c2"]
end
end

x = reshape(collect(1:100), 10, 10)
y = reshape(collect(101:200), 10, 10)
a = collect(1:10)
Expand Down