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

Drop TimeZones.jl dependendency #114

Open
oxinabox opened this issue Oct 14, 2021 · 8 comments
Open

Drop TimeZones.jl dependendency #114

oxinabox opened this issue Oct 14, 2021 · 8 comments

Comments

@oxinabox
Copy link
Member

TimeZones v1.6 chagned the representaion of FixedTimeZone (big improvement on GC).
This upgrade means can't load the old Checkpoints without loading the right version of TimeZones.jl.
Which normally we could do via Pkg.activate(jlsofile), before we do using TimeZones and JLSO.load(jlsofile)
But Pkg.activate has no effect due to the fact that a newer version of TimeZones.jl has already been loaded when JLSO.jl was loaded.

TimeZones.jl, unlike our other deps, contains a basic data type that is commonly used for data we want to store.

JLSO has a indirect dependency on it via Memento.
It should be reasonable enough to drop Memento and just use the stdlib logger.
We only log in a a few places:

warn(LOGGER, e)

info(LOGGER, "Upgrading $src -> $dest")

@oxinabox
Copy link
Member Author

oxinabox commented Nov 2, 2021

@rofinn why the 👎 ?

@rofinn
Copy link
Member

rofinn commented Nov 8, 2021

Cause only logging in a few places isn't a reason to switch logging backends. The primary benefit of Memento is the flexible configuration system which is still applicable here. For example, consider loading hundreds of JLSO files which need to be upgraded as part of some analysis. If you find that output overly verbose you can easily disable it along with our other libraries when configuring Memento. AFAIK, that's less easy with the base logging and would be a completely different process in most of our script.

Honestly, I think the issue is with how Julia's environment system works. There are numerous issues/bugs with the current Pkg.activate, package loading and serialized Julia objects. For example, Pkg.activate has very weird behaviour if you already have some packages loaded, which isn't JLSO specific. This isn't going to be easily resolved by simply changing a JLSO dependency, that's just side-stepping the main issue here. FWIW, I think we should look into deserializing Julia objects on a worker process with the correct environment (e.g., addprocs(exeflags="--project"), remotecall(f, ...)). That would largely resolve the issue of having different package versions loaded so long as the communication between the manager and worker doesn't depend on it.

@oxinabox
Copy link
Member Author

oxinabox commented Nov 9, 2021

This isn't going to be easily resolved by simply changing a JLSO dependency, that's just side-stepping the main issue here.

While this is true, TimeZones.jl is the only upstream dependency of JLSO that defines an object type that is likely to be serialized in a JLSO.
So side-stepping it here seems like it would give us a lot of benefit, with very little effort.

In contrast to worker processes (which I agree is a good idea) which is much more effort.

@rofinn
Copy link
Member

rofinn commented Oct 21, 2022

Memento doesn't explicitly depend on TimeZones.jl anymore.

@rofinn rofinn closed this as completed Oct 21, 2022
@iamed2
Copy link
Member

iamed2 commented Oct 24, 2022

We don't depend on the version of Memento that dropped TimeZones.jl though, and TimeZones.jl is still in the Project.toml

@iamed2 iamed2 reopened this Oct 24, 2022
@rofinn
Copy link
Member

rofinn commented Oct 25, 2022

Hmm, good point. We should bump the Memento version. We just have a compat entry for TimeZones as a test dependency which is unrelated to Memento (we use timezones in our test files and the ZonedDateTime struct format changed). I don't remember, what's the best practice is for test dependency compat entries?

@iamed2
Copy link
Member

iamed2 commented Oct 25, 2022

I don't remember, what's the best practice is for test dependency compat entries?

I'm not sure. I don't know if the behaviour of Pkg.test is documented either.

@oxinabox
Copy link
Member Author

For test dependencies, their compat goes in [compat] (like for main dependencies) and they get listed in [extras] and [target] test.
We do this a lot, in many packages.
Though we are not as strict about always including the compat entry for test dependencies, we really should be.

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 a pull request may close this issue.

3 participants