Skip to content

[BUG] _validate_dir requires every configured directory to be writable — breaks read-only container and input directories at 2.0.0 #138

Description

@dadamsncsa

Summary

Config._validate_dir on main requires every configured directory to be
writable. Three of the seven it is applied to are legitimately read-only, so a
site install with an admin-managed container directory — and the read-only input
directory that #121's changelog entry advertises — both fail at Config()
construction. v1.0.0 has no such check, so this breaks working setups on
upgrade.

Reproducer 1 — admin-managed container directory

Our Lmod module for the deployed llmflux/1.0.0 sets

setenv("LLMFLUX_CONTAINERS_DIR", "/sw/llmhub/llmflux/containers/1.0.0")

pointing at a versioned, service-account-owned directory holding
llm_processor.sif, mode 0755, deliberately not user-writable. Against main
@ 64686c9:

$ LLMFLUX_CONTAINERS_DIR=/sw/llmhub/llmflux/containers/1.0.0 \
    llmflux run --model Qwen2.5-0.5B-Instruct --input p.jsonl --output o.json \
                --account <acct> --partition <part>
Traceback (most recent call last):
  File ".../llmflux/cli.py", line 1054, in main
    return args.func(args)
  File ".../llmflux/cli.py", line 295, in _run_command
    config = Config()
  File ".../llmflux/core/config.py", line 219, in __init__
    self.containers_dir = str(self._validate_dir(Path(containers_dir or os.getenv('LLMFLUX_CONTAINERS_DIR') or self.workspace / "containers").expanduser().resolve()))
  File ".../llmflux/core/config.py", line 257, in _validate_dir
    raise OSError(f"Cannot use directory '{path}': {e}") from e
OSError: Cannot use directory '/sw/llmhub/llmflux/containers/1.0.0': Directory is not writable: /sw/llmhub/llmflux/containers/1.0.0

An unhandled traceback, before any work is attempted.

Reproducer 2 — the read-only input directory #121 advertises

import os, tempfile
os.environ['LLMFLUX_WORKSPACE'] = tempfile.mkdtemp()
os.environ['LLMFLUX_DATA_INPUT_DIR'] = '<a 0555 directory>'
from llmflux.core.config import Config
Config()
# OSError: Cannot use directory '...': Directory is not writable: ...

The [Unreleased] changelog entry for #121 says the input/output split exists so
a user can have "a read-only input location on one filesystem and results
written to another"
. That configuration is currently fatal.

The change

v1.0.0, src/llmflux/core/config.py:214 — no validation:

self.containers_dir = containers_dir or os.getenv('LLMFLUX_CONTAINERS_DIR') or str(self.workspace / "containers")

main, src/llmflux/core/config.py:240-258:

def _validate_dir(self, path: Path) -> Path:
    """Ensure a directory exists and is writable, creating it if necessary."""
    try:
        path.mkdir(parents=True, exist_ok=True)
        if not os.access(path, os.W_OK):
            raise PermissionError(f"Directory is not writable: {path}")
    except (OSError, PermissionError) as e:
        raise OSError(f"Cannot use directory '{path}': {e}") from e
    return path

applied at config.py:211,214-219 to workspace, data_dir, data_input_dir,
data_output_dir, models_dir, logs_dir and containers_dir.

Which of the seven actually need write access

Directory Written to by llmflux? Writability check correct?
workspace yes yes
data_dir yes yes
data_output_dir yes yes
logs_dir yes yes
containers_dir no — only $CONTAINERS_DIR/llm_processor.sif is read no
data_input_dir no — read-only input is a documented use case (#121) no
models_dir no, where a site stages a shared read-only cache no

Impact

Any site shipping LLMFlux as a module with an admin-managed container directory
breaks on upgrade to 2.0.0 — module load llmflux && llmflux run fails at
Config(). We have two clusters in that position (x86_64 A100/H200 and aarch64
GH200); both fail. The only workaround is a user-writable directory holding a
symlink to the deployed .sif, which defeats the point of the module setting
LLMFLUX_CONTAINERS_DIR at all and pushes per-user setup onto every user.

Suggested fix

Validate for the access each directory actually needs:

self.containers_dir  = str(self._validate_dir(..., need_write=False))
self.data_input_dir  = str(self._validate_dir(..., need_write=False))
self.models_dir      = str(self._validate_dir(..., need_write=False))
# workspace, data_dir, data_output_dir, logs_dir keep need_write=True

where need_write=False means: must exist (do not mkdir a path the user
cannot create), and must be readable and traversable — os.access(path, os.R_OK | os.X_OK). That keeps the diagnostic value the check was added for
while permitting the read-only configurations the docs and site deployments rely
on. Exempting containers_dir alone unbreaks the module case, but
data_input_dir still needs it for #121's stated use case.

Tests worth adding: containers_dir and data_input_dir at 0555 construct
successfully; logs_dir at 0555 still raises; a non-existent
containers_dir raises with a message naming the variable rather than being
silently created.

Affected

main @ 64686c9 (version = "2.0.0", unreleased). Not present in v1.0.0.
Happy to open a PR for the need_write variant if that shape is agreeable.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions