-
Notifications
You must be signed in to change notification settings - Fork 17
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
bring loggers in sync and add multiproc capabilities #1885
Conversation
paired PR: cmu-delphi/delphi-epidata#1254 |
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.
Reviewed this PR rather than the epidata one since it has a little more commits. Looks good with a few small caveats!
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.
Mostly have some questions and want to understand the multiprocessing stuff better
if level == logging.INFO: | ||
logger.info(*args, **kwargs) | ||
if level == logging.WARN: | ||
logger.warn(*args, **kwargs) |
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.
Do we need debug or exception (or others) here too? Seems like a situation where sublogger needs to inherit from Logger, but instead of writing to a file it needs to send a message to that queue. Otherwise, we need to hard duplicate Logger's interface or make sure downstream users know that multiprocessing subloggers != regular loggers. Am I reading this 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.
we only need the methods we use, and right now, nobody is using debug or exception. i had a TODO here to make it more obvious to anyone who came looking that they could add their own, but the current pylint settings disallowed that...
this is sort of a proxy or facade for a Logger, and it doesnt need to duplicate the whole interface. someone using a SubLogger got there by providing an existing Logger object in the first place, so hopefully they have some idea that its still wrapped up in there.
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.
...i caved and added some other levels
logger.debug('stopping thread') | ||
|
||
self.thread = threading.Thread(target=logger_thread_worker, | ||
name="LoggerThread__"+logger.name) |
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.
I don't quite follow the purpose of this thread... it seems like it should interact with self.sublogger in some way, no? Could you explain a bit?
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.
the thread doesnt really "interact" with the sublogger; the sublogger acts indirectly upon the thread. the thread is spawned in the background with a handle to a queue and a handle to the provided Logger, and it just logs anything that shows up on the queue. the sublogger is used by things outside the thread (or even outside the process!) to enqueue things to be logged.
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.
I see! So verifying that I get it:
- So msg_queue.get() will block until it receives a message (because no timeout)
- SubLoggers send messages to that queue that that thread will pick up
- Eventually, that thread is terminated by the STOP message
- We let the thread close fully with thread.join below
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.
yep!
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.
One doc suggestion, but otherwise lgtm!
Co-authored-by: Dmitry Shemetov <[email protected]>
this does a few things, including:
bringing the
logger.py
files from https://github.com/cmu-delphi/delphi-epidata/ and https://github.com/cmu-delphi/covidcast-indicators/ back in sync. (there will be equivalent PRs in both repositories.)larger functional differences are now applied to both:
covidcast-indicators
didnt log PIDsdelphi-epidata
didnt always respect output filenames properly (though it seems we didnt actually trigger this in practice; look at the collapsible section below for how to see this for yourself).differentiated the uncaught exception handlers a bit
added multiprocess-safe logging constructs, which should be helpful in Add multiprocessing to the Quidel indicator #1881 and in the future. (see collapsible section below for sample usage)
to reproduce bad outfile behavior:
python3
interpreter, check those files, and repeat:two example usages of multiprocessing-compatible logger:
multiprocessing
directly, requires explicit termination of the LoggerThread (after ensuring the pool jobs have completed), and requires thesublogger
to be instantiated before the worker and bound in an accessible scope (it must be used as though it is local to the worker, it cannot be passed as an argument to pool methods)