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

Add caching options #412

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

Add caching options #412

wants to merge 10 commits into from

Conversation

dbrakenhoff
Copy link
Collaborator

This allows overriding caching behavior using nlmod.config.NLMOD_CACHE_OPTIONS. This allows you to turn off the hash checks, and optionally activate an explicit dataset coordinate comparison. This allows some more flexibility when running into issue #389.

Recommended usage:

import nlmod

# ... define some ds

# temporarily turn off dataset coordinate hash check and turn on explicit check
with nlmod.config.cache_options(
    dataset_coords_hash=False,
    explicit_dataset_coordinate_comparison=True,
):
    nlmod.read.ahn.get_ahn(ds, cachename="ahn", cachedir=".")

add config submodule

  • define NLMOD_CACHE_OPTIONS
  • set defaults (identical to current caching defaults)
  • add context manager for cache options
  • add functions to set, get and reset options.

in cache.py

  • add check for global setting in NLMOD_CACHING_OPTIONS
  • add logging when global config modifies default
  • do not check hashes if global option overrides cache decorator setting
  • add explicit dataset coordinate check, useful if hash checks are disabled.

- define NLMOD_CACHE_OPTIONS
- set defaults (identical to current caching defaults)
- add context manager for cache options
- add functions to set, get and reset options.
- add check for setting NLMOD_CACHING_OPTIONS
- add logging when global config modifies default
- do not check hashes if global option overrides cache decorator setting
- add explicit dataset coordinate check, useful if hash checks are disabled.
@dbrakenhoff dbrakenhoff self-assigned this Feb 26, 2025
@dbrakenhoff dbrakenhoff linked an issue Feb 26, 2025 that may be closed by this pull request
@dbrakenhoff dbrakenhoff added the caching All caching related issues label Feb 26, 2025
Copy link
Collaborator

@OnnoEbbens OnnoEbbens left a comment

Choose a reason for hiding this comment

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

Just one comment and then you are good to go, should be an easy fix

@dbrakenhoff
Copy link
Collaborator Author

Good point about only invalidating the cache. This is the output now when you turn on the explicit coordinate check:
image

While writing this I realize that the caching should be as quiet as possible, so these ERROR messages should be downgraded to INFO I think. I think it is good to know why the cache is being invalidated once you choose this explicit route? But I could also live with DEBUG level. Lemme know what you think.

@dbrakenhoff
Copy link
Collaborator Author

I went for full debug level logging in the excplicit check.

Also, I removed "area" from the data set variables in ds_contains(). I think this was put there for resampling AHN data, but I think this variable can be easily computed if it does not yet exist.

I think this is good to go now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caching All caching related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache is never used because hashes do not match
3 participants