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

Stop using Memento #116

Closed
wants to merge 1 commit into from
Closed

Stop using Memento #116

wants to merge 1 commit into from

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Nov 9, 2021

Closes #114 which will make dealing with data that was saves with different version of TimeZones.jl better.

Will also avoid other TimeZones related suffering hit by JLSO uses (cc @ablaom) like:

  • slow load times
  • build-time cache breaking (I think those are all fixed now though)

Test.@test_logs is more annoying to work with than Memento.@test_warn see JuliaLang/julia#43016.
This because @warn exception=e doesn't convert e to a string as part of message so @test_logs doesn't accept a regex to apply to it.
Because of that we only test if a warning was given rather than what it says.

The change to the package code (rather than the test code) is pretty minimal

This PR should only be merged if @rofinn is convinced.

@oxinabox oxinabox requested a review from rofinn November 9, 2021 14:56
@@ -59,8 +58,6 @@ export JLSOFile
const READABLE_VERSIONS = semver_spec("1, 2, 3, 4")
const WRITEABLE_VERSIONS = semver_spec("3, 4")

const LOGGER = getlogger(@__MODULE__)
__init__() = Memento.register(LOGGER)

Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change

@@ -82,7 +82,7 @@ function Base.read(io::IO, ::Type{JLSOFile})
)
)
catch e
warn(LOGGER, e)
@warn exception=e
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@warn exception=e
@warn exception = e

Comment on lines +3 to 5
supported || throw(ArgumentError(
"Unsupported version ($version). Expected a value between ($valid_versions)."
))
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
supported || throw(ArgumentError(
"Unsupported version ($version). Expected a value between ($valid_versions)."
))
return supported || throw(
ArgumentError(
"Unsupported version ($version). Expected a value between ($valid_versions).",
),
)

@@ -53,7 +53,7 @@ function getindex(jlso::JLSOFile, name::Symbol)
decompressing_buffer = decompress(jlso.compression, buffer)
return deserialize(jlso.format, decompressing_buffer)
catch e
warn(LOGGER, e)
@warn exception=e
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@warn exception=e
@warn exception = e

@@ -16,13 +16,13 @@ upgrade step. Won't be needed once the v2 file format is dropped.
The project generated is accurate, except for duplicated names (no UUID) and custom branchs.
"""
function upgrade(src, dest)
info(LOGGER, "Upgrading $src -> $dest")
@info "Upgrading" src dest
jlso = open(io -> read(io, JLSOFile), src)
write(dest, jlso)
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
write(dest, jlso)
return write(dest, jlso)

"Failed to construct an environment with the provide package version " *
"($pkgs): $e.\n Falling back to simply adding the packages."
end
"\nFalling back to simply adding the packages.",
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
"\nFalling back to simply adding the packages.",
"\nFalling back to simply adding the packages.",

end
"\nFalling back to simply adding the packages.",
pkgs,
exception=e
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
exception=e
exception = e

Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
Serialization = "9e88b42a-f829-5b0c-bbe9-9e923198166b"

[compat]
BSON = "0.2.4, 0.3"
CodecZlib = "0.6, 0.7"
FilePathsBase = "0.7, 0.8, 0.9"
Memento = "0.10, 0.11, 0.12, 0.13, 1"
TimeZones = "0.9, 0.10, 0.11, 1"
TimeZones = "0.9, 0.10, 0.11, ~1.0, ~1.1, ~1.2, ~1.3, ~1.4, ~1.5"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated to this PR.
CI has actually been broken for ages.
https://github.com/invenia/JLSO.jl/actions/runs/1428133005

This is because we have specimen files that were saved with TimeZones prior to v1.5.

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #116 (64e0663) into master (04b83b3) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   97.67%   97.66%   -0.02%     
==========================================
  Files           6        6              
  Lines         215      214       -1     
==========================================
- Hits          210      209       -1     
  Misses          5        5              
Impacted Files Coverage Δ
src/JLSO.jl 100.00% <ø> (ø)
src/file_io.jl 94.44% <0.00%> (ø)
src/metadata.jl 100.00% <100.00%> (ø)
src/serialization.jl 100.00% <100.00%> (ø)
src/upgrade.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04b83b3...64e0663. Read the comment docs.

@rofinn
Copy link
Member

rofinn commented Nov 9, 2021

I'm still not convinced, but then I actively dislike Julia's base logging system. Since we haven't changed the core file format in a while, we aren't likely to hit those upgrade logs. I still think Memento probably shouldn't depend directly on TimeZones.jl, and it should be an optional dependency instead. If you manage to get CI passing then I'll just merge.

https://github.com/invenia/Memento.jl/blob/master/src/records.jl#L160

@rofinn
Copy link
Member

rofinn commented Nov 9, 2021

Here's an alternate fix which makes TimeZones.jl an optional dependency of Memento.jl invenia/Memento.jl#182.

  1. Now the TimeZones.jl mismatch would only happen if you explicitly loaded that before loading JLSO.jl (same issue we'd have regardless
  2. Having mismatched Memento versions should be fine because folks shouldn't be trying to serialize logging constructs anyways.

@rofinn
Copy link
Member

rofinn commented Oct 18, 2022

I'm gonna close this since the specific issue was fixed in Memento.jl, feel free to re-open if you disagree.

@rofinn rofinn closed this Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop TimeZones.jl dependendency
2 participants