diff --git a/.issueflows/03-solved-issues/issue786_original.md b/.issueflows/03-solved-issues/issue786_original.md new file mode 100644 index 00000000..c5a50fb2 --- /dev/null +++ b/.issueflows/03-solved-issues/issue786_original.md @@ -0,0 +1,49 @@ +# Issue #786: list_instruments() still logs loader-probe warnings on every call + +Source: https://github.com/jepegit/cellpy/issues/786 + +## Original issue text + +## Problem +`cellpy.readers.data_structures.instrument_configurations()` is the right way to +discover loaders (it's what `print_instruments` uses), but for an app it has two +rough edges: + +1. **It logs a `WARNING` per non-loader module on every call** — `config_declarations`, + `contract`, `hooks`, `declarations`, `registry`, `testing`, and `custom` + ("Missing instrument definition file"). A GUI calling it at startup has to + bracket it with a log-level bump to keep the console clean. +2. **The result isn't display-ready** — ids like `maccor_txt` / `pec_csv` need a + human label, and the raw file **suffix/extension** isn't returned alongside the + models, so apps keep their own label + extension map. + +## Suggestion +A quiet, app-facing helper, e.g.: + +```python +cellpy.list_instruments() +# -> [{"id": "maccor_txt", "label": "Maccor (text)", +# "models": ["one","two","three", ...], "suffixes": [".txt"]}, ...] +``` + +that (a) does not emit warnings for skipped/non-loader modules, and (b) includes a +human label and the raw suffix(es). This would let apps build an instrument picker ++ ingestion form directly from cellpy. + + +--- +*Found while building [cellpy-simple-gui](https://github.com/cellpy/cellpy-simple-gui) on cellpy 2.1.0.post1. Full write-up with all items: [CELLPY_PAINPOINTS.md](https://github.com/cellpy/cellpy-simple-gui/blob/main/CELLPY_PAINPOINTS.md).* + +## Comments (curated summary) + +- **Additional tasks**: + - Make probe/discovery skips quiet: calling `cellpy.list_instruments()` (or `instrument_configurations()`) must not emit a `WARNING` per skipped non-loader module on every call (repro still fails on 2.1.1.post1). +- **Clarifications / constraints**: + - The app-facing shape (`id` / `label` / `models` / `suffixes`) from the first #786 landing is **done** — do not rework labels/suffixes. + - Warnings currently hit the **root** logger (`WARNING:root:`), so silencing only `logging.getLogger("cellpy")` is ineffective. + - Preferred fixes (either OK): log expected discovery skips (no `DataLoader`, or `custom` with no definition file) at `DEBUG`; and/or have `list_instruments()` swallow probe failures so the public helper is quiet by contract. + - Remaining pain points (metadata read, per-instrument schema, figure theming) stay in #791 / related issues — out of scope here. +- **Superseded / retracted**: + - Original item (2) “result isn't display-ready” — solved by the shipped `list_instruments()` helper; this reopen is **warnings-only**. + +_Note: this section is an interpretive summary of the comment thread, not a verbatim dump. Source comments: 1, last comment by @jepegit on 2026-07-29._ diff --git a/.issueflows/03-solved-issues/issue786_plan.md b/.issueflows/03-solved-issues/issue786_plan.md new file mode 100644 index 00000000..ae145d09 --- /dev/null +++ b/.issueflows/03-solved-issues/issue786_plan.md @@ -0,0 +1,68 @@ +# Issue #786 — plan (warnings-only reopen) + +## Goal + +Make `cellpy.list_instruments()` (and ideally discovery via `instrument_configurations()`) quiet for **expected** loader-probe skips, so apps can call it at startup without bracketing the root logger. Keep the existing `id` / `label` / `models` / `suffixes` API unchanged. + +## Constraints + +- Warnings-only reopen: do **not** redesign labels/suffixes (already shipped in #796). +- Out of scope: #799 / #800 / #801 / other #791 items. +- Preserve behaviour of real loaders that fail for environmental reasons when used through normal load paths; only discovery noise should change. +- Public surface already documented in `docs/getting_started/agents.md` — update only if the quiet contract needs an explicit note. + +### Prior art + +- `cellpy.list_instruments()` in [`cellpy/readers/data_structures.py`](cellpy/readers/data_structures.py) — already raises `logging.getLogger("cellpy")` to `ERROR` during the scan; **misses** bare `logging.warning(...)` calls that go to the **root** logger. +- `InstrumentFactory.create_all()` in the same module — on create failure logs `logging.warning(f"Could not create loader for {key}: {e}")` except `local_instrument` (debug). This is the noise source. +- Module already has `logger = logging.getLogger(__name__)` at top of `data_structures.py` — unused by `create_all`. +- Tests: `test_list_instruments_shape_and_known_entry` / `test_list_instruments_is_quiet` in [`tests/test_instrument_registering.py`](tests/test_instrument_registering.py). The quiet test only asserts **Python `warnings`**, not **logging WARNING** — so it currently passes while the bug remains. +- Toolbox / graph: nothing relevant for logging quieting. + +## Approach + +1. **Classify expected discovery skips in `InstrumentFactory.create_all`** + - If create fails because the module has no `DataLoader`, or `custom` raises “Missing instrument definition file…”, log at **DEBUG** via the module `logger` (not bare `logging.warning`). + - Other failures (e.g. missing ODBC for a real SQL loader) stay at **WARNING**, but also via the module logger so they are filterable under `cellpy.readers.data_structures` rather than `root`. + +2. **Make `list_instruments()` quiet by contract** + - Add `create_all(..., quiet=False)`; `list_instruments()` calls `create_all(quiet=True)` so *all* probe failures during that scan are DEBUG (including env-missing real loaders). + - Drop or shrink the ineffective `getLogger("cellpy").setLevel(ERROR)` wrapper once the above is in place (keep `warnings.catch_warnings` only if something still emits Python warnings). + +3. **Harden the test** + - Replace/extend `test_list_instruments_is_quiet` to use `caplog` at WARNING on the root logger **and** `cellpy.readers.data_structures`, asserting no “Could not create loader” records when calling `list_instruments()`. + - Keep the shape/label test as-is. + - Mark the quiet regression `@pytest.mark.essential` if Tier-1 should guard it (small, fast). + +4. **Docs / HISTORY** — one Unreleased bullet that #786 warnings reopen is fixed (close step); optional one-liner in agents.md that `list_instruments()` is quiet at WARNING+. + +## Files to touch + +| Path | Change | +|------|--------| +| `cellpy/readers/data_structures.py` | `create_all(quiet=…)`; expected skips → DEBUG; use module logger; `list_instruments` uses `quiet=True` | +| `tests/test_instrument_registering.py` | Caplog-based quiet assertion (and essential marker if agreed) | +| `HISTORY.md` | Unreleased fix bullet (at close) | +| `docs/getting_started/agents.md` | Optional one-line quiet-contract note | + +## Test strategy + +```bash +uv run pytest tests/test_instrument_registering.py -q +uv run pytest -m essential +``` + +Manual smoke (matches issue repro): + +```python +import logging, cellpy +logging.basicConfig(level=logging.INFO) +cellpy.list_instruments() # no WARNING:root Could not create loader ... +``` + +## Open questions + +1. **Should `instrument_configurations()` / `print_instruments` also go quiet for expected non-loader skips?** + **Recommended: yes** for the “no DataLoader / missing custom def” class (DEBUG), so `print_instruments` stops spam too; keep WARNING for unexpected real-loader failures unless `quiet=True`. +2. **Essential marker on the quiet test?** + **Recommended: yes** — tiny, guards the reopen regression that shipped incompletely once already. diff --git a/.issueflows/03-solved-issues/issue786_status.md b/.issueflows/03-solved-issues/issue786_status.md new file mode 100644 index 00000000..9ea30c75 --- /dev/null +++ b/.issueflows/03-solved-issues/issue786_status.md @@ -0,0 +1,22 @@ +# Issue #786 — status + +- [x] Done + +## What's done + +- Plan accepted; build on `cursor/786-list-instruments-warnings-5e1f`. +- `InstrumentFactory.create_all(quiet=...)`: expected discovery skips → DEBUG on module logger; unexpected failures → WARNING on module logger (not root). +- `list_instruments()` uses `create_all(quiet=True)` (removed ineffective `cellpy` logger bump). +- Caplog quiet regression + `@pytest.mark.essential`; agents.md quiet-contract note. +- Verified: `list_instruments()` emits 0 "Could not create loader" WARNINGs; `instrument_configurations()` only warns for real env failures (e.g. missing ODBC). +- HISTORY promoted to `[2.1.1.post3]`; planned release tag `v2.1.1.post3` on `master` after merge. +- PR: https://github.com/jepegit/cellpy/pull/807 (#807) + +## Remaining work + +- None. + +## Release + +- Planned tag: `v2.1.1.post3` (git-tag derived; `post` bump from `v2.1.1.post2`) +- Cut from `master` via `gh release create v2.1.1.post3 --target master --generate-notes` after squash-merge. diff --git a/HISTORY.md b/HISTORY.md index 2c6507d2..9e696144 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -2,6 +2,15 @@ ## [Unreleased] +## [2.1.1.post3] - 2026-07-30 + +Post-release of 2.1.1 — silence loader-discovery WARNING spam for apps. + +* `list_instruments()` is quiet by contract: expected loader-probe skips + (no `DataLoader` / missing custom def) log at DEBUG on the module logger; + the scan uses `create_all(quiet=True)` so apps no longer see + `WARNING:root: Could not create loader …` on every call. (#786) + ## [2.1.1.post2] - 2026-07-30 Post-release of 2.1.1 — collected summary facet y-axis controls for apps. diff --git a/cellpy/readers/data_structures.py b/cellpy/readers/data_structures.py index 82c827aa..65ee24ab 100644 --- a/cellpy/readers/data_structures.py +++ b/cellpy/readers/data_structures.py @@ -851,10 +851,24 @@ def get_registered_kwargs(self): def get_registered_builder(self, key): return self._builders.get(key, None) - def create_all(self, **kwargs): + @staticmethod + def _is_expected_discovery_skip(key: str, exc: BaseException) -> bool: + """True for probe failures that are normal during module discovery.""" + if key == "local_instrument": + return True + msg = str(exc) + if "has no attribute 'DataLoader'" in msg or 'has no attribute "DataLoader"' in msg: + return True + if "Missing instrument definition file" in msg: + return True + return False + + def create_all(self, quiet: bool = False, **kwargs): """Create all the instrument loader modules. Args: + quiet: if True, log every create failure at DEBUG (used by + :func:`list_instruments` so apps get a silent listing). **kwargs: sent to the initializer of the loader class. Returns: @@ -876,10 +890,11 @@ def create_all(self, **kwargs): loaders[key] = models except Exception as e: - if key == "local_instrument": - logging.debug(f"Could not create loader for {key}: {e}") + message = f"Could not create loader for {key}: {e}" + if quiet or self._is_expected_discovery_skip(key, e): + logger.debug(message) else: - logging.warning(f"Could not create loader for {key}: {e}") + logger.warning(message) return loaders @staticmethod @@ -1077,19 +1092,13 @@ def list_instruments() -> List[Dict[str, Any]]: [{"id": "maccor_txt", "label": "Maccor (text)", "models": ["default", "ZERO", ...], "suffixes": [".txt"]}, ...] """ - cellpy_logger = logging.getLogger("cellpy") - previous_level = cellpy_logger.level - cellpy_logger.setLevel(logging.ERROR) - try: - with warnings.catch_warnings(): - warnings.simplefilter("ignore") - factory = InstrumentFactory() - for instrument, settings in find_all_instruments().items(): - if instrument not in LOADERS_NOT_READY_FOR_PROD: - factory.register_builder(instrument, settings) - loaders = factory.create_all() - finally: - cellpy_logger.setLevel(previous_level) + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + factory = InstrumentFactory() + for instrument, settings in find_all_instruments().items(): + if instrument not in LOADERS_NOT_READY_FOR_PROD: + factory.register_builder(instrument, settings) + loaders = factory.create_all(quiet=True) listing: List[Dict[str, Any]] = [] for loader_id, models in loaders.items(): diff --git a/docs/getting_started/agents.md b/docs/getting_started/agents.md index 7b9b24f2..bb635673 100644 --- a/docs/getting_started/agents.md +++ b/docs/getting_started/agents.md @@ -134,8 +134,9 @@ Suggested layering: `collection.plot(y_ranges={"coulombic_efficiency": [0, 110]})` (Plotly). `share_y=True` / `match_axes=True` restores a shared scale. - **Instrument picker for free.** `cellpy.list_instruments()` returns - `[{"id", "label", "models", "suffixes"}, ...]` (quiet — no per-module - warnings), ready to drive an import form. + `[{"id", "label", "models", "suffixes"}, ...]` and is quiet by contract + (probe/discovery skips stay at DEBUG — no `WARNING` spam on the root + logger), ready to drive an import form. - **Keep the console quiet.** cellpy logs through the `cellpy` logger; raise its level in an app you want silent: `logging.getLogger("cellpy").setLevel(logging.ERROR)`. Suppress one-off diff --git a/tests/test_instrument_registering.py b/tests/test_instrument_registering.py index 8e1b8bf3..aecebf01 100644 --- a/tests/test_instrument_registering.py +++ b/tests/test_instrument_registering.py @@ -209,13 +209,26 @@ def test_list_instruments_shape_and_known_entry(): assert by_id["pec_csv"]["label"] == "PEC (CSV)" -def test_list_instruments_is_quiet(): - """No per-non-loader-module warnings escape (the whole point, #786).""" +@pytest.mark.essential +def test_list_instruments_is_quiet(caplog): + """No per-non-loader-module warnings escape (the whole point, #786). + + Probe failures historically used bare ``logging.warning`` (root logger); + silencing only ``logging.getLogger("cellpy")`` was not enough. Assert on + logging WARNING records, not just Python ``warnings``. + """ import warnings import cellpy with warnings.catch_warnings(record=True) as caught: warnings.simplefilter("always") - cellpy.list_instruments() + with caplog.at_level(logging.WARNING): + cellpy.list_instruments() assert not caught, [str(w.message) for w in caught] + probe_noise = [ + r + for r in caplog.records + if r.levelno >= logging.WARNING and "Could not create loader" in r.getMessage() + ] + assert not probe_noise, [r.getMessage() for r in probe_noise]