Enable passing custom logging services#211
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the logging subsystem to allow users to plug in custom logging backends via configuration (addressing the request for supporting services like MLflow) by introducing a common LogWriter interface and constructing writers dynamically via resolve_callable.
Changes:
- Added a
LogWriterABC and refactored W&B/Neptune writers to implement it. - Updated
Loggerto instantiate logging backends fromcfg["logger"](dict form) with deprecation warnings for string aliases. - Updated docs/API references and bumped project version to
5.4.0.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ruff.toml |
Adjusts Ruff ignores (adds global ignore for B027). |
rsl_rl/utils/wandb_log_writer.py |
Refactors W&B writer into WandbLogWriter implementing LogWriter. |
rsl_rl/utils/neptune_log_writer.py |
Refactors Neptune writer into NeptuneLogWriter implementing LogWriter. |
rsl_rl/utils/logger.py |
Creates writers from config via resolve_callable, adds deprecation handling and capability-based uploads. |
rsl_rl/utils/log_writer.py |
Introduces LogWriter ABC with optional no-op hooks for artifacts/config. |
rsl_rl/utils/__init__.py |
Exports LogWriter and built-in log writers for resolution via resolve_callable. |
pyproject.toml |
Bumps package version to 5.4.0. |
docs/guide/overview.rst |
Adds documentation note about passing custom classes via config. |
docs/guide/configuration.rst |
Updates configuration docs for new logger config shape and deprecations. |
docs/api/utils.rst |
Updates API docs to point to the new log writer modules. |
CONTRIBUTORS.md |
Adds a contributor entry. |
CITATION.cff |
Updates cited software version to 5.4.0. |
Comments suppressed due to low confidence (1)
ruff.toml:43
- Adding
B027to the global ignore list may hide legitimate “empty method” issues elsewhere. Since this seems driven by intentional no-op methods inLogWriter, consider removing the global ignore and instead making those methods explicit no-ops (e.g.,return None) or adding targeted# noqa: B027on the specific definitions.
ignore = ["B006",
"B007",
"B027",
"B028",
"ANN401",
"D100",
"D203",
"D213",
"D413",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MarkusPorti
left a comment
There was a problem hiding this comment.
I think this looks pretty good.
I'm personally not a fan of mapping strings to actual classes, because it is hard to trace where it actually gets called and can make debugging hard, but in this case it's an easy and straight forward solution.
A more "clean" solution could be done using the Registry Pattern. But maybe this is overkill 😅
|
Hmm, good point. The reason why I did it here is that algorithms and models are handled the same way currently. But maybe there is a better way... Does the registry pattern also allow users to pipe their custom class to the library without modifying it? Thanks a lot for the review! |
|
From my understanding, the Registry Pattern works by getting access to all subclasses or "users" of an annotation. Then the rsl_rl user would just need to implement the class and it would be "automatically" available and could be resolved via the config. |
|
Sounds interesting, I will have a look when I find the time. Thanks again! |
Adresses Issue #207