-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use QueueHandler
for Pipeline
logging
#489
Conversation
@@ -72,8 +70,8 @@ def generate( | |||
per input in `inputs`.""" | |||
pass | |||
|
|||
@cached_property | |||
def generate_parameters(self) -> List[inspect.Parameter]: | |||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the from functools import cached_property
can be removed too, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! BTW I removed it because in some environments pickle
was not able to serialize it.
@@ -45,6 +45,8 @@ class GenerateEmbeddings(Step): | |||
llm: LLM | |||
|
|||
def load(self) -> None: | |||
"""Loads the `LLM` used to generate the embeddings.""" | |||
super().load() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all the steps inheriting from Step
need to actually call the super().load()
isn't it better if we just add it as a separate method that runs within the post_init
? i.e. model_post_init
on _Step
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly we can't do that :( We need to create the logger after having called setup_logging
in each process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we then just do step.load()
and then step.setup_logging
? But to manage that within the Pipeline
instead of having to always add that as part of the load
method :/
Description
This PR updates the logging setup for the local
Pipeline
to use aQueueHandler
and aQueueListener
(a thread in the main process). With this setup, child processes send log messages using amultiprocessing.Queue
and theQueueHandler
to theQueueListener
, which will be in charge of handling the log messages.This change was mainly motivated to improve the logging messages in environments like Google Colab and IPython Notebooks, in which not all the logging messages (from the child processes) were being displayed.