This operation could fail on an HPC compute node with a limited tmp filesystem. It should respect our time_series_directory kwarg, env variable, or something new.
❯ I am reviewing this PR: https://github.com/Sienna-Platform/PowerSystems.jl/pull/1785
When _from_file_sienna is called, where does path come from? I'm concerned about it being hard-coded for a tmpdir.
Read 1 file, ran 6 shell commands
path isn't hard-coded. It's whatever the caller passed to from_file, which picks the .sn branch from the file extension. The temp directory comes one step later, and your concern holds there.
Where path comes from
src/openapi/file_io.jl on the PR branch:
function from_file(path::AbstractString; system_kwargs...) # :421
...
elseif IS.is_sienna_archive(path)
return _from_file_sienna(path; system_kwargs...) # :425
function _from_file_sienna(path::AbstractString; system_kwargs...) # :471
dir = IS.extract_sienna_archive(path)
sys = _from_file_directory(dir; system_kwargs...)
_load_sienna_extras!(sys, dir)
Where the extraction goes
IS4's src/sienna_archive.jl:89-93:
function extract_sienna_archive(path::AbstractString)
...
dir = mktempdir()
The extraction target is a bare mktempdir(): the system temp dir, with no way to change it. That causes three problems:
1. It ignores time_series_directory and SIENNA_TIME_SERIES_DIRECTORY. Those options exist so users can keep the large .h5 off a small or node-local /tmp, which matters on HPC. The extraction writes the whole archive, .h5 included, to tempdir() before either option is looked at.
2. The default writable read keeps two extra copies on disk. import_document.jl:711-714 passes time_series_directory to IS.open_deserialized_infrastore_store. That function copies the extracted .h5 and .sqlite into time_series_directory (or tempdir() again) and opens the copy (infrastore.jl:76-86). So you end up with the archive, the extracted copy, and the working copy. The extracted copy is never used again, but nothing deletes it until the process exits (mktempdir's cleanup-at-exit).
3. With time_series_read_only = true, the store is opened in place inside that temp dir. The comment at file_io.jl:472-473 is correct here: a .sn has its .sqlite, so has_catalog is true and the store isn't copied. But the live store then sits in an unconfigurable /tmp path and disappears at process exit.
Suggested fix
The fix is small and belongs mostly in IS:
- Give extract_sienna_archive a directory keyword.
- Have _from_file_sienna pass it time_series_directory, falling back to the env var and then tempdir(), the same way TimeSeriesManager resolves it.
- In the writable case, extract into a scoped mktempdir() do ... end. The store has been copied out by the time from_openapi returns, so the extraction can be deleted right away. Only the read-only case needs it to persist.
This operation could fail on an HPC compute node with a limited tmp filesystem. It should respect our time_series_directory kwarg, env variable, or something new.
LLM debug notes: