Conversation
cevian
temporarily deployed
to
internal-contributors
GitHub Actions
Inactive
projects/pgai/pgai/logger.py
Outdated
| def get_logger(name: str = "") -> logging.Logger: | ||
| """Get a logger instance with the pgai namespace. | ||
|
|
||
| Args: | ||
| name: The logger name, which will be prefixed with 'pgai.' | ||
|
|
||
| Returns: | ||
| A Logger instance with the appropriate namespace | ||
| """ | ||
| if name: | ||
| logger_name: str = f"pgai.{name}" | ||
| else: | ||
| logger_name: str = "pgai" | ||
|
|
||
| return logging.getLogger(logger_name) |
There was a problem hiding this comment.
I don't understand how this is any different to "just" using logging.getLogger(__name__)?
There was a problem hiding this comment.
it enforces the pgai prefix
There was a problem hiding this comment.
The prefix is always pgai (except in __main__.py), because all files are in the pgai module.
projects/pgai/pgai/logger.py
Outdated
| def set_level(level: int | str) -> None: | ||
| """Set the log level for all pgai loggers. | ||
|
|
||
| This does not affect the root logger or any other loggers outside | ||
| the pgai namespace. | ||
|
|
||
| Args: | ||
| level: The logging level (e.g., logging.INFO, logging.DEBUG) | ||
| or a string level name ('INFO', 'DEBUG', etc.) | ||
| """ | ||
| if isinstance(level, str): | ||
| numeric_level: int = getattr(logging, level.upper(), logging.INFO) | ||
| else: | ||
| numeric_level = level | ||
|
|
||
| logging.getLogger("pgai").setLevel(numeric_level) |
There was a problem hiding this comment.
I'm not sure I understand the utility of this. The caller of this could just as well use logging.getLogger("pgai").setLevel() themselves, which is the standard way of doing this in python logging.
There was a problem hiding this comment.
yeah I can get rid of this
projects/pgai/pgai/logger.py
Outdated
|
|
||
| @staticmethod | ||
| def default_renderer(msg: str, kwargs: dict[str, Any]) -> str: | ||
| return f"{msg} >>> {json.dumps(kwargs)}" |
There was a problem hiding this comment.
This fails when the vectorizer worker runs, because json.dumps can't serialize UUID.
| RendererType = Callable[[str, dict[str, Any]], str] | ||
|
|
||
|
|
||
| class StructuredMessage: |
There was a problem hiding this comment.
I don't understand why we're introducing the StructuredMessage class. I am aware that we had structured logging with structlog, but I don't understand if we're just trying to replicate what structlog had, or provide some functionality for our users.
There was a problem hiding this comment.
This is really mostly for internal usage to be able to keep some of the structured data we already specified in the output
There was a problem hiding this comment.
This change breaks logging for the install subcommand (no logs appear).
cevian
temporarily deployed
to
internal-contributors
GitHub Actions
Inactive
cevian
force-pushed
the
mat/remove_structlog
branch
from
45eec89 to
0e61ee2
Compare
cevian
temporarily deployed
to
internal-contributors
GitHub Actions
Inactive
It is considered best practice for Python libraries to use the standard logger package.
cevian
force-pushed
the
mat/remove_structlog
branch
from
0e61ee2 to
9111bc0
Compare
cevian
temporarily deployed
to
internal-contributors
GitHub Actions
Inactive
cevian
force-pushed
the
mat/remove_structlog
branch
from
9111bc0 to
a910278
Compare
cevian
temporarily deployed
to
internal-contributors
GitHub Actions
Inactive
cevian
temporarily deployed
to
internal-contributors
GitHub Actions
Inactive
alejandrodnm
force-pushed
the
main
branch
2 times, most recently
from
35e80a5 to
f15011e
Compare
2 participants
It is considered best practice for Python libraries to use the standard logger package: https://docs.python.org/3/howto/logging.html#library-config.