Let LAYUP_CACHE_DIR set the default data location (#448) - #484
Let LAYUP_CACHE_DIR set the default data location (#448)#484matthewholman wants to merge 3 commits into
Conversation
layup writes roughly 1.6 GB of SPICE kernels, observatory codes and debiasing
tables to a cache directory chosen by pooch.os_cache("layup"), which sits under
the user's home directory. On a cluster the home directory is frequently a
different and smaller partition than the one layup is installed on, so that is
the wrong place. Raised by Hanno Rein in #443.
A cache_dir argument already overrode it per call, and `layup bootstrap` has
--cache, but neither helps: the point of the issue is that the DEFAULT should be
settable once rather than threaded through every call site.
Adds layup.utilities.cache_location.default_cache_dir(), which returns
$LAYUP_CACHE_DIR when set and pooch.os_cache("layup") otherwise, and routes all
ten call sites across eight files through it. Precedence is now an explicit
cache_dir argument, then the environment variable, then the OS cache.
Deliberate details:
* A blank or whitespace-only value falls back to the default rather than
writing into the current directory, since that is a shell accident and not
a request.
* A leading ~ is expanded, because an environment variable is not shell-
expanded when a process reads it.
* The directory is not created as a side effect of asking where it is; pooch
still creates it on download, which is the previous behaviour.
* `import pooch` is dropped from the six files that only used it for this.
Verified end to end through the CLI, not just the helper: with the variable set,
`layup bootstrap --help` reports the overridden path as its default; without it,
the original. Seven tests cover the four cases and assert the unset default is
byte-for-byte what layup used before.
Full suite: 484 passed. (Three tests appear to fail unless the layup under test
is first on PATH -- `layup` dispatches by shelling out to `layup-<verb>`
executables found there, so a second installation silently wins. They pass with
PATH set correctly.)
| Every place in layup that needs the default calls :func:`default_cache_dir`, so | ||
| setting the variable moves all of it. An explicit ``cache_dir`` argument still | ||
| wins over the environment variable, which in turn wins over the OS cache. | ||
| """ |
There was a problem hiding this comment.
I think it would be helpful for this to be condensed into a shorter comment this is easier to understand. It references tickets when I think in 1-2 years will be confusing.
| Returns | ||
| ------- | ||
| pathlib.Path | ||
| ``$LAYUP_CACHE_DIR`` if that variable is set to a non-empty value, with |
There was a problem hiding this comment.
Maybe this won't happen but if I had one job running on an old cache and wanted to try a new one if they were running in the same environment the caches would get crossed? Maybe that's okay but maybe somewhere that needs to be mentioned in the documentation?
Two points from @mschwamb's review. **The module comment was too long and leaned on ticket numbers.** Her objection was that references to issues will be confusing to read in a year or two, which is fair -- they are shorthand for whoever is in the thread this week, not for someone reading the source later. The docstring now says what the variable does and what the precedence is, with no issue numbers and about half the words. **She asked what happens if two jobs run in the same environment with different caches.** Worth answering precisely rather than waving at it: the variable is read on each call rather than captured at import, so a process can change it mid-run, and two processes with different values are independent. But it is a process setting inherited from the shell, so two jobs launched from the SAME shell share whatever that shell exports -- which is exactly the case she described. Set it per job rather than exporting it once. That is now in the module docstring and, per her suggestion, in the documentation: the bootstrap section of the landing page gains the export line, notes that every command reads it, and carries a note about the shared-shell case. Also merges main, so this branch has the landing page that #478 added.
|
Both addressed in On the comment length and the ticket references — you were right, and the second half especially. Issue numbers are shorthand for whoever is in the thread this week; a year from now they are a lookup someone has to do to read a docstring. It is now about half the length, says what the variable does and what the precedence is, and cites no issues. On two jobs crossing caches — worth answering precisely rather than waving at it, because the answer is "mostly fine, except in exactly the case you described":
That is now in the module docstring, and per your suggestion in the documentation: the bootstrap section of the landing page gains the export line, notes that every command reads it, and carries a note about the shared-shell case. The branch also merges |
Closes #448.
The problem
layupwrites roughly 1.6 GB of SPICE kernels, observatory codes and debiasing tables to the directorypooch.os_cache("layup")picks, which lives under the user's home directory. On a cluster the home directory is frequently a different — and smaller — partition from the one layup is installed on, so that is the wrong place for it. Raised by @hannorein in #443.A
cache_dirargument already overrode this per call, andlayup bootstraphas--cache. Neither addresses the issue, which is that the default should be settable once rather than threaded through every call site.The change
Adds
layup.utilities.cache_location.default_cache_dir():and routes all ten call sites across eight files through it. Precedence is now an explicit
cache_dirargument, then the environment variable, then the OS cache — so nothing that worked before changes.Four details that were decided rather than defaulted:
export LAYUP_CACHE_DIR=is a shell accident, not a request to scatter 1.6 GB wherever you happened to be standing.~is expanded. An environment variable is not shell-expanded when a process reads it, soLAYUP_CACHE_DIR=~/datawould otherwise create a directory literally named~.poochstill creates it on download, which is the previous behaviour.import poochis dropped from the six files that only imported it for this.Verification
Tested through the CLI, not just the helper — with the variable set,
layup bootstrap --helpreports the overridden path as its default; without it, the original:Seven tests cover the four cases, including that the unset default is byte-for-byte what layup used before this change.
Full suite: 484 passed.
One thing worth knowing for anyone testing this
Three tests (
test_comet_output,test_predict_output,test_get_onsky_data_output) appear to fail unless the layup under test is first onPATH. That is not a flake and not related to this change:layupdispatches by scanningPATHforlayup-<verb>executables and shelling out to them, so any second installation silently wins over the one you just built. It cost me some time here, so it seems worth writing down. WithPATHset correctly all three pass.That mechanism might deserve its own issue — a CLI that resolves its own subcommands through
PATHwill pick up a stale sibling install without saying so — but it is out of scope here.