Skip to content

feat(logging): shared logger skeleton with Rule extension - #46

Open
timtreis wants to merge 14 commits into
scverse:mainfrom
timtreis:feat/shared-logger
Open

feat(logging): shared logger skeleton with Rule extension#46
timtreis wants to merge 14 commits into
scverse:mainfrom
timtreis:feat/shared-logger

Conversation

@timtreis

@timtreis timtreis commented Jun 18, 2026

Copy link
Copy Markdown
Member

Shared logger for scverse packages. One scverse parent logger + single handler (rich if installed, else plain); package loggers are children, so verbosity/rich/rules are controlled centrally.

from scverse_misc.logging import get_logger, config, Rule

log = get_logger("spatialdata")        # real logging.Logger, child of "scverse"
config.verbosity = "info"              # central, all packages
config.add_rule(MyRule())              # 0..N rules

Sole extension point is Rule (keep/rewrite). Ships Elapsed/Deep universal rules (on by default, no-ops until used). get_logger(name, timed=True) opts into scanpy-style time=/deep=/.hint() + a datetime return.

Packages keep their behavior with a local rule

spatialdata-plot — report the public entry point instead of internal helpers:

from contextvars import ContextVar
_ctx: ContextVar[str] = ContextVar("ctx", default="")

class ContextPrefix(Rule):
    def rewrite(self, message, record):
        ctx = _ctx.get()
        return f"{ctx}: {message}" if ctx else message

config.add_rule(ContextPrefix())   # render_* entry points set _ctx

decoupler — always-on timestamp:

from datetime import datetime

class Timestamp(Rule):
    def rewrite(self, message, record):
        return f"{datetime.now():%H:%M:%S} | {message}"

config.add_rule(Timestamp())

design proposal for discussion

timtreis and others added 2 commits June 18, 2026 22:18
One `scverse` parent logger + single handler (rich auto-detected, else plain),
package loggers as children, central control via `config`. Sole extension point
is `Rule` (keep/rewrite); ships `Elapsed`/`Deep` universal rules on by default.
Opt-in `get_logger(name, timed=True)` adds scanpy-style `time=`/`deep=`/`.hint()`
and a datetime return.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.67%. Comparing base (de3484b) to head (2f19905).

Files with missing lines Patch % Lines
src/scverse_misc/logging.py 97.48% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #46      +/-   ##
==========================================
+ Coverage   92.63%   93.67%   +1.03%     
==========================================
  Files          11       12       +1     
  Lines         584      743     +159     
==========================================
+ Hits          541      696     +155     
- Misses         43       47       +4     
Files with missing lines Coverage Δ
src/scverse_misc/__init__.py 100.00% <100.00%> (ø)
src/scverse_misc/datasets/__init__.py 100.00% <ø> (ø)
src/scverse_misc/logging.py 97.48% <97.48%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

timtreis and others added 2 commits June 18, 2026 22:47
The logging module shipped its checks inside `if __name__ == "__main__"`,
which CI never runs, so the 255 new lines had 0% coverage; the same block
plus the module body also failed ruff (missing annotations/docstrings,
E702) and would fail strict mypy.

- annotate the public surface (`_emit`, level methods, `verbosity`,
  `get_logger` via @overload so `timed=True` narrows to `_TimedLogger`);
  add the missing `Rule` docstrings; type `_rules: list[Rule]`;
  `__getattr__ -> Any` for transparent delegation
- trim `_emit` to the documented `time=`/`deep=` surface (the old
  `**kwargs` passthrough never passed strict mypy)
- remove the `__main__` self-check, promote it to tests/test_logging.py
  (18 tests; logging.py coverage 0% -> 99%)

Verified locally: ruff check + format, mypy --strict, pytest all pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
logging.py imports rich.logging lazily in the rich-handler branch. rich is
an optional runtime dep, but mypy needs it to resolve RichHandler (else
import-not-found + a no-any-return on the Handler return). Matches the
existing pattern of listing typecheck-only deps (pytest, sphinx) here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@timtreis
timtreis marked this pull request as ready for review June 18, 2026 20:52
Comment thread src/scverse_misc/logging.py Outdated
Comment on lines +108 to +150
class _Config:
"""Central logging configuration; the singleton instance is :data:`config`."""

def __init__(self) -> None:
self._parent = logging.getLogger(_ROOT)
self._parent.setLevel(logging.WARNING)
self._parent.propagate = False # one handler here; don't double-log via root
self._rules: list[Rule] = [Elapsed(), Deep()] # universal defaults; order matters
self._install(_make_handler(_rich_available()))

def _install(self, handler: logging.Handler) -> None:
for r in self._rules:
handler.addFilter(r)
self._parent.addHandler(handler)

@property
def verbosity(self) -> str | int:
"""Central level for all scverse loggers. Set with a name (``"info"``) or int."""
return logging.getLevelName(self._parent.level)

@verbosity.setter
def verbosity(self, level: str | int) -> None:
self._parent.setLevel(level.upper() if isinstance(level, str) else level)

def use_rich(self, enabled: bool = True) -> None:
"""Force the rich (``True``) or plain (``False``) handler."""
for h in list(self._parent.handlers):
self._parent.removeHandler(h)
self._install(_make_handler(enabled))

def add_rule(self, rule: Rule) -> None:
self._rules.append(rule)
for h in self._parent.handlers:
h.addFilter(rule)

def remove_rule(self, rule: Rule) -> None:
if rule in self._rules:
self._rules.remove(rule)
for h in self._parent.handlers:
h.removeFilter(rule)


config = _Config()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we integrate that with the global scverse-misc config handling?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially (except the rule logic), yeah. But I think then we should promote pydantic as a core dep, no? Otherwise we need a fallback for that too here

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this would make logging depend on the settings dependency group which would be ok IMO.

@timtreis timtreis Jun 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we're about to create a complicated net of various dependency groups that depend on dependency groups. Could we maybe be somewhat opinionated here? 👀 Like do we really want to add a new dependency group for every feature we add? While that's at least only slightly annoying, making them inter-dependent feels a lot

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since logging has no additional dependencies, logging/settings would be a single one.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it even depend on anndata?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#40

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but why is it not part of the datasets dependency group?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent question : ) I didn't get around to reviewing that PR before it was merged, but my assumption would be that if a package is downloading anndata files, it will be using them and will depend on anndata anyway, so scverse-misc shouldn't need anndata anywhere in its dependencies. Same with spatialdata.

@flying-sheep flying-sheep Jun 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scverse-misc now hard-depends on anndata

Actually let’s undo that, we now have a circular dependency. Not good!

Like do we really want to add a new dependency group for every feature we add?

Yes, very much so!

And why does this PR add more hard dependencies? I’m a fan of keeping things modular. Adding more and more hard deps is exactly the “slippery slope” I feared in making this package, and keeping it modular (and adding good tests for the powerset of that modularity!) keeps that somewhat in check.

making them inter-dependent feels a lot

Let’s not then. Let’s add a logging extra that depends on pydantic and not pydantic-settings.

Comment thread src/scverse_misc/logging.py Outdated
Comment thread src/scverse_misc/logging.py Outdated
Comment thread src/scverse_misc/logging.py Outdated
timtreis and others added 6 commits June 19, 2026 13:47
- make TimedLogger public (returned from get_logger, meant to be subclassed)
- config.rich settable property for consistency with config.verbosity
- rename _Config._parent -> _root (it manages the scverse root logger)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Promote pydantic-settings + python-dotenv to hard dependencies (anndata
always pulls scverse-misc[settings] in anyway), and rebuild the logging
config on the shared Settings base instead of a bespoke _Config.

- verbosity/rich become validated Settings fields: env-var loading
  (SCVERSE_MISC_*), override/reset, and level validation come for free.
- rules stay bespoke (add_rule/remove_rule) — they aren't settings.
- _root/_rules derive from the live logger/handler, so there's no
  pydantic private-attr timing to manage and the test suite is unchanged.
- keep the (now empty) settings extra for anndata <0.13 back-compat.
- drop the dead suppress(ImportError) around Settings in __init__.

Addresses review feedback on scverse#46 (integrate logger with global config
handling; promote pydantic; rich settable).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/scverse_misc/logging.py Outdated
if use_rich:
from rich.logging import RichHandler

return RichHandler(show_path=False, show_time=False) # rich renders the level itself

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make it write to stderr by default. Do we want that?

Suggested change
return RichHandler(show_path=False, show_time=False) # rich renders the level itself
return RichHandler(console=Console(stderr=True), show_path=False, show_time=False) # rich renders the level itself

timtreis and others added 4 commits August 18, 2026 16:51
Address review feedback on scverse#46 (@flying-sheep): logging must not add hard
dependencies and must not depend on pydantic-settings.

- Drop anndata, pydantic-settings, python-dotenv from hard deps; restore the
  settings extra; anndata is now consumer-provided (kills the circular dep).
- logging config is two-tier via a shared _RuleAccess mixin + module helpers
  (_canonical_level, _reinstall, _add_rule/_remove_rule): full tier subclasses
  Settings (env-vars, override/reset) when pydantic-settings is present, else a
  stdlib reduced tier keeps verbosity/rich/rules. Single source of mechanics, so
  the tiers can't diverge.
- Re-guard the Settings export in __init__ with suppress(ImportError).
- Fix reduced-tier bugs surfaced by review: validate verbosity (no silent
  disable on bad int), rich defaults to None, _rules mirrors handler.filters
  (no resurrect on toggle), rich setter no-ops when unchanged.
- Guard tests/settings/* with importorskip(pydantic_settings); add a
  forced-fallback test; document the anndata loader requirement.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view

Per @flying-sheep: logging must not pull pydantic-settings. Rest the
validated config tier on plain pydantic.BaseModel behind a new `logging`
extra; without pydantic the stdlib tier keeps identical behavior.

Also fold in review fixes clustered on the reviewers' themes:
- rich handler now logs to stderr (Console(stderr=True)), matching the
  plain handler and scanpy (grst's request)
- forcing config.rich=True without rich raises a clear ImportError
- tier parity: canonicalize the pydantic verbosity default and coerce
  rich to bool in the stdlib tier, so both tiers agree pre-assignment
- test group installs `rich` so the rich handler tier is exercised
- sink fixture restores rich + handler stream on teardown
- CHANGELOG: move datasets/logging entries to [Unreleased] (0.0.9 is
  released), fix typo

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts:
#	pyproject.toml
#	tests/settings/test_sphinx.py
…amination

importlib.reload(mod) rebound the module's classes, so a later
isinstance(log, TimedLogger) compared against a stale class object and
failed (order/worker-dependent; tripped py3.12/3.13 under xdist). Load a
fresh, isolated module copy instead of reloading the shared one.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@timtreis
timtreis requested a review from flying-sheep August 18, 2026 19:16

@flying-sheep flying-sheep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the design: I don’t like that rewrite actually changes the stored message – logging should separate context, message, and formatting.

E.g. people attaching a JSON log handler to decoupler’s logger should get {"message": ..., "extra": {"timestamp": ...}} or so (maybe flattened extra), not {"message": "2026-... | ..."}.

So I think this should be a two step process:

  1. add a hook that adds minimal data to extra. It should avoid any string wrangling and just do the absolute minimum to amend the data that’s stored (which also happens to be faster)
  2. configure the formatter for text logging to include the extra fields. We should use style="{" as nobody remembers how to use ancient C style formatting while we’re all using f-strings daily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants