From ae86a37a9b9b0e3a2024274844a058f6324a378e Mon Sep 17 00:00:00 2001 From: mgrani Date: Thu, 6 Aug 2026 17:21:58 +0200 Subject: [PATCH] docs(remote): state what listing an object-store remote costs, and fix ls --help F4, plus a bug found while doing it. Documented on `GET /remote/ls` (docs/source/http.md, new "Listing Cost" section) and in `owi remote ls --help`, with both measurements: 84.1 s to return 0 datasets from the s3a mirror against 7.1 s to return 584 from LEXIS. The cost tracks the size of the store, not the size of the answer, so it is slowest when it has least to say. Consumers are told to budget minutes, to cache, and not to use `ls` as a liveness probe. The cheap existence check asked for already exists: FileBasedRepository.status does one fs.exists per access level and never enumerates -- and F7 is what stopped it raising for exactly these backends. The docs now point at POST /remote/doctor/status for health views. A cheap *count* is deliberately not offered, and the measurement is the reason rather than an assumption: the 84 s call returned zero datasets, so load_metadata was never reached and the whole 84 s was listdir/ls traversal. A "shallow count" skipping per-dataset metadata would cost the same 84 s while advertising itself as cheap. Serving a count without enumerating needs a maintained index, which is a different feature. F8, found by running the command whose help I was editing: `owi remote ls --help` crashed with MarkupError -- the specifier syntax `[/]` in the docstring is also valid Rich markup, and Typer renders docstrings as markup. Help for the most-used remote command produced a traceback. Reproduced at v5.7.0, so it predates this work. Nothing had ever invoked --help in a test. tests/owilix/cli/test_help_renders.py now sweeps all 57 commands; it fails at v5.7.0 on exactly `remote ls` and passes everywhere else. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017phbSd6D8u4iEQsCAPEw6s --- ..._pull_and_mirror_operability_2026-08-04.md | 60 ++++++++++++++- docs/source/http.md | 39 +++++++++- owilix/cli/remote.py | 19 ++++- tests/owilix/cli/test_help_renders.py | 75 +++++++++++++++++++ 4 files changed, 186 insertions(+), 7 deletions(-) create mode 100644 tests/owilix/cli/test_help_renders.py diff --git a/docs/review/remote_pull_and_mirror_operability_2026-08-04.md b/docs/review/remote_pull_and_mirror_operability_2026-08-04.md index b830fab..2188f45 100644 --- a/docs/review/remote_pull_and_mirror_operability_2026-08-04.md +++ b/docs/review/remote_pull_and_mirror_operability_2026-08-04.md @@ -127,6 +127,28 @@ synchronous consumer must therefore either cache or expect minutes; ours set a *Suggested:* document the cost on `/remote/ls`, and consider a cheap existence/count path that does not enumerate. +**Answered in v5.8.0. Documented, existence check pointed at, count declined — +with a reason.** + +- The cost and both measurements are now on `GET /remote/ls` in + `docs/source/http.md` (§ Listing Cost) and in `owi remote ls --help`, + including the advice not to use `ls` as a liveness probe and not to set a + short timeout against an object-store remote. +- **The cheap existence check already existed and now works.** + `FileBasedRepository.status` does one `fs.exists` per access level and never + enumerates, and F7 is what stopped it raising for exactly these backends. So + the non-enumerating probe consumers want is `POST /remote/doctor/status`, + which is where the docs now send them. +- **A cheap *count* is not offered, deliberately.** Counting datasets in an + object store means listing the keys under the prefix; there is no cheaper + primitive to wrap. The measurement above is the evidence rather than an + assumption: the 84 s call returned *zero* datasets, so `load_metadata` was + never reached and the whole 84 s went into `listdir`/`ls` traversal. A + "shallow count" that skipped per-dataset metadata would therefore have cost + the same 84 s while advertising itself as cheap. Serving a count without + enumerating requires a maintained index or manifest — a different feature, + and not one `ls` can be adapted into. + --- ## F5 — `remote doctor` gives HTTP callers nothing to read @@ -193,12 +215,22 @@ discovered backend, including ones added later. ## F6 — Smaller things +**Both fixed and released in v5.8.0.** + - **Two bare `except:`** clauses in `core/tasks/remote.py` swallow - `KeyboardInterrupt` and `SystemExit` along with everything else. + `KeyboardInterrupt` and `SystemExit` along with everything else. *Fixed:* + `except Exception` with a debug log; the expected case is a missing README or + stats file, which is not worth a warning but is worth being able to see. - **`ask_yes_no` assumes a TTY**: `termios.tcgetattr(sys.stdin)` raises `Inappropriate ioctl for device` when stdin is not a terminal. Reached whenever a non-interactive caller omits `--yes`; detecting `sys.stdin.isatty()` - and declining (or erroring clearly) would be kinder than a traceback. + and declining (or erroring clearly) would be kinder than a traceback. *Fixed:* + declines and prints what to pass instead. Declining rather than accepting is + deliberate — `default='y'` describes what Enter means for a human at a prompt, + not consent from a caller that was never asked. `--yes` still short-circuits + ahead of the check. Only `core/manager/ui.py` had this; the second + `ask_yes_no` in `compat/core_base.py` uses `Prompt.ask` and never touches + termios. - **Not a finding, recorded to prevent re-reporting:** `files` is parsed with `ast.literal_eval` in `query_utils.py`, not bare `eval`. Older versions do evaluate `files` directly. Checked against the tags while releasing v5.6.0: @@ -211,6 +243,30 @@ discovered backend, including ones added later. --- +## F8 — `owi remote ls --help` crashed + +**Severity: low, but it had been broken for a long time. Fixed and released in +v5.8.0.** + +Found while documenting F4, by running the command whose help was being edited: + +``` +MarkupError: closing tag '[/]' at position 74 doesn't match any open tag +``` + +Typer renders docstrings as Rich markup, and the specifier syntax in that +docstring — `[:|...][/]` — is *also* valid Rich markup, so +`[/]` parsed as a closing tag with nothing open. Help for the most-used +remote command therefore produced a traceback instead of help. Reproduced +against the v5.7.0 tag, so this predates the current work by some distance. + +*Fixed:* the docstring is raw and the literal brackets are escaped. Nothing had +ever invoked `--help` in a test, which is why it went unnoticed; +`tests/owilix/cli/test_help_renders.py` now sweeps **all 57 commands**, and +fails at v5.7.0 on exactly `remote ls`. + +--- + ## What we changed on our side For the record, so this review is not read as "all of it is yours": diff --git a/docs/source/http.md b/docs/source/http.md index 782751f..5fee40e 100644 --- a/docs/source/http.md +++ b/docs/source/http.md @@ -89,7 +89,8 @@ Local endpoints: Remote endpoints: -- `GET /remote/ls` +- `GET /remote/ls` (**can take minutes on object-store remotes** — see + [Listing cost](#listing-cost)) - `POST /remote/search` - `POST /remote/pull` - `POST /remote/push` @@ -177,12 +178,46 @@ owilix http serve For local-only testing, `OWILIX_HTTP_S3_NO_AUTH=true` disables S3 facade credential checks. +## Listing Cost + +`GET /remote/ls` enumerates. For a LEXIS repository the catalogue is queried and +the answer comes back quickly; for an object-store remote (`s3a`/`s3`) the store +itself is walked, so **the cost tracks the size of the store rather than the size +of the answer** — and it is slowest when it has least to say. Measured against a +full OWI cluster: + +| specifier | time | result | +| --- | --- | --- | +| `owi-up/collectionName=main` (s3a) | **84.1 s** | 0 datasets | +| `lexis/collectionName=main` | 7.1 s | 584 datasets | + +Consequences for a synchronous caller: + +- **Budget minutes, not seconds.** A 30 s client timeout against an object-store + remote will fail every time, and it fails in the way least likely to be + diagnosed correctly — as a connection error rather than as a slow answer. +- **Cache the result** if you are rendering it repeatedly (a dashboard panel, a + health view). The answer changes far more slowly than the cost of asking. +- **Do not use `ls` as a liveness probe.** Use `POST /remote/doctor/status`, + which reports per-access-level existence with a handful of prefix checks and + does not enumerate. It answers "is this remote reachable, and does it hold + anything at each access level" — which is what a health light needs. + +There is deliberately **no cheap dataset *count*** endpoint. A count over an +object store requires listing the keys under the prefix; there is no cheaper +primitive to expose. The measurement above is the evidence: that 84 s call +returned *zero* datasets, so no per-dataset metadata was ever fetched and the +time went entirely into walking the store. A "count without enumerating" would +have to be served from a maintained index or manifest rather than from the store, +which is a different feature and not something `ls` can be made to do. + ## Known Limits - HTTP endpoint authorization is preview-level; use only on localhost or behind external access control. - Jobs are in-process only and disappear after restart. - Job cancellation and durable progress records are not implemented. -- `remote/ls` pagination currently slices after fetching backend results. +- `remote/ls` pagination currently slices after fetching backend results — so a + page request costs the same as the full listing (see [Listing cost](#listing-cost)). - The S3 facade has been smoke-tested; broader compatibility with `aws s3`, `boto3`, and `rclone` remains follow-up work. diff --git a/owilix/cli/remote.py b/owilix/cli/remote.py index 8862308..a4f3aed 100644 --- a/owilix/cli/remote.py +++ b/owilix/cli/remote.py @@ -76,11 +76,11 @@ def ls( files_glob: Optional[str] = typer.Option(None, "--files", "-f", help="List files matching glob pattern (e.g., '**/*.parquet')"), file_details: bool = typer.Option(False, "--file-details", help="Show file details (size) - slower"), ): - """ + r""" List remote datasets matching SPECIFIER. - + Specifier format: - [:|latest|#|..][/] + \[:|latest|#|..]\[/] Scope aliases: all - all configured remote sources @@ -107,6 +107,19 @@ def ls( owi remote ls all/collectionName=main;zone=IT4ILexisV2,OWILRZZONE owi remote ls all/title*=.*Web.*; owi remote ls all/id=abc123 --files "**/*.parquet" + + Cost: + Listing an object-store remote (s3a/s3) scans it, so the time tracks the + size of the store rather than the size of the answer -- and it is slowest + when it has least to say. Measured on a full OWI cluster: + + owi-up/collectionName=main (s3a) 84.1 s -> 0 datasets + lexis/collectionName=main 7.1 s -> 584 datasets + + Budget minutes, or cache, for any synchronous caller. To ask only whether + a remote is reachable and holds anything, use 'owi remote doctor' + (POST /remote/doctor/status), which checks for existence per access level + instead of enumerating. """ cli_ctx: CLIContext = ctx.obj cli_ctx.fields = fields diff --git a/tests/owilix/cli/test_help_renders.py b/tests/owilix/cli/test_help_renders.py new file mode 100644 index 0000000..922be4f --- /dev/null +++ b/tests/owilix/cli/test_help_renders.py @@ -0,0 +1,75 @@ +"""`--help` must render for every command. + +`owi remote ls --help` crashed with + + MarkupError: closing tag '[/]' at position 74 doesn't match any open tag + +because the specifier syntax in its docstring -- ``[:...][/]`` +-- is also valid Rich markup, and Typer renders docstrings as markup. The help +for the most-used remote command was unreadable, and nothing failed to say so: +no test invoked ``--help``. + +Sweeping every command rather than pinning the one that broke, since the trap is +in the docstrings themselves and applies to any of them. +""" +import pytest +from typer.testing import CliRunner + +from owilix.cli import app + +runner = CliRunner() + + +def _command_paths(typer_app, prefix=()): + """Every (sub)command path in the CLI, as argv fragments.""" + import typer.main + + click_command = typer.main.get_command(typer_app) + yield from _walk(click_command, prefix) + + +def _walk(command, prefix): + subcommands = getattr(command, "commands", None) + if not subcommands: + return + for name, sub in subcommands.items(): + path = (*prefix, name) + yield path + yield from _walk(sub, path) + + +ALL_COMMANDS = sorted(_command_paths(app)) + + +def test_command_discovery_found_something(): + """Guard the guard: an empty parametrisation would pass silently.""" + assert len(ALL_COMMANDS) > 20, f"only discovered {len(ALL_COMMANDS)} commands" + assert ("remote", "ls") in ALL_COMMANDS + + +@pytest.mark.parametrize("path", ALL_COMMANDS, ids=lambda p: " ".join(p)) +def test_help_renders(path): + result = runner.invoke(app, [*path, "--help"]) + + assert result.exit_code == 0, ( + f"`owi {' '.join(path)} --help` failed:\n{result.output}" + ) + # A MarkupError is *raised*, so it surfaces as a non-zero exit above; check + # the text too in case it is ever caught and printed instead. + assert "MarkupError" not in result.output + + +def test_remote_ls_help_shows_the_specifier_syntax_literally(): + """The brackets are syntax to copy, not markup to interpret.""" + result = runner.invoke(app, ["remote", "ls", "--help"]) + + assert result.exit_code == 0 + assert "[/]" in result.output + + +def test_remote_ls_help_documents_the_listing_cost(): + """F4: the cost of listing an object-store remote must be discoverable.""" + result = runner.invoke(app, ["remote", "ls", "--help"]) + + assert "84.1 s" in result.output + assert "doctor" in result.output, "point the reader at the cheap probe" -- 2.51.2