Improve logging #235
Improve logging #235
Conversation
stellasia
force-pushed
the
fix/debuging-exprience
branch
from
80b0647 to
e4342e8
Compare
| @@ -101,10 +103,14 @@ def from_config( | |||
| cls, config: AbstractPipelineConfig | dict[str, Any], do_cleaning: bool = False | |||
| ) -> Self: | |||
| wrapper = PipelineConfigWrapper.model_validate({"config": config}) | |||
| logger.debug( | |||
| f"PIPELINE_RUNNER: instantiate Pipeline from config type: {wrapper.config.template_}" | |||
There was a problem hiding this comment.
Minor but this should probably be instantiating and not instantiate
| return value | ||
|
|
||
|
|
||
| prettyfier = Prettyfier() |
There was a problem hiding this comment.
Why instantiate an instance of the Prettyfier class here and then import the instance? Why not just import the class and then create an instance of it where it's used?
There was a problem hiding this comment.
The main reason is to keep things simple when logging, we already need an extra step (calling the function), if we also have to instantiate the class every time we use a logger, I think it makes the code more complicated.
We could also have a function like this:
def prettify(value):
prettifer = Prettifyer()
return prettifier(value)
and import this function when logging, but since the Prettifyer class is always the same, I though it would be easier like this. But I'm happy to change if you prefer the function approach or another one.
There was a problem hiding this comment.
I'm happy to leave it for now as I don't have a good intuition for what would be the best approach in this situation. It might be something worth changing in a future refactor though.
| DEFAULT_MAX_STRING_LENGTH: int = 200 | ||
|
|
||
|
|
||
| class Prettyfier: |
There was a problem hiding this comment.
Do we have any unit tests for this class? I think testing the nested functionality would be especially useful
Description
Truncate long lists and strings in logs. The max length of list of string is configurable using env vars.
Type of Change
Complexity
Complexity: Low
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):