Skip to content

bring loggers in sync and add multiproc capabilities #1254

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

Merged
merged 4 commits into from
Aug 14, 2023
Merged

Conversation

melange396
Copy link
Collaborator

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:

    • the version in covidcast-indicators didnt log PIDs
    • nor did it have the env var trick to get debug-level output
    • the version in delphi-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

    • and reduced the log level for messages from threads exiting cleanly. this is mostly because multiprocessing.Manager processes start and stop threads often.
  • added multiprocess-safe logging constructs, which should be helpful in Add multiprocessing to the Quidel indicator covidcast-indicators#1881 and in the future. (see collapsible section below for sample usage)


to reproduce bad outfile behavior:
  • first, in your shell:
curl https://github.com/raw/cmu-delphi/covidcast-indicators/main/_delphi_utils_python/delphi_utils/logger.py -o ci_logger.py

curl https://github.com/raw/cmu-delphi/delphi-epidata/dev/src/common/logger.py -o de_logger.py
  • then follow these instructions: toggle the commented-out-ness of the imports below, remove files foo.txt and bar.txt, paste this into your python3 interpreter, check those files, and repeat:
import de_logger as a_logger
#import ci_logger as a_logger

foo = a_logger.get_structured_logger('foo', filename='foo.txt')
bar = a_logger.get_structured_logger('bar', filename='bar.txt')
foo.info("hi")
bar.info("oh hello")
two example usages of multiprocessing-compatible logger:
  • this approach uses multiprocessing directly, requires explicit termination of the LoggerThread (after ensuring the pool jobs have completed), and requires the sublogger 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)
import multiprocessing
from delphi.epidata.common.logger import get_structured_logger, LoggerThread

logger = get_structured_logger('sample_1')

lt = LoggerThread(logger)
sl = lt.get_sublogger()
sl.info("test")

def worker_1(message):
  # do something here
  sl.info(message)
  # do somethine else here

num_processes = 5
with multiprocessing.Pool(num_processes) as pool:
  pool.apply_async(worker_1, args=["abcd"])
  pool.apply_async(worker_1, args=["efgh"])
  pool.close()
  pool.join()

lt.stop()
  • this approach uses the context manager to provide a pool and logger, and the logger can be used in arguments to pool methods. this is a little bit less lightweight (the multiprocessing.Manager() it uses will spawn its own subprocess and a number of threads within it) but it is cleaner and more straightforward.
from delphi.epidata.common.logger import get_structured_logger, pool_and_threadedlogger

logger = get_structured_logger('sample_2')

def worker_2(message, sublogger):
  # do something here
  sublogger.info(message)
  # do somethine else here

num_processes = 5  # optional argument passed to Pool constructor
with pool_and_threadedlogger(logger, num_processes) as (pool, sublogger):
  pool.apply_async(worker_2, args=["asdf", sublogger])
  pool.apply_async(worker_2, args=["jkl;", sublogger])
  pool.apply_async(worker_2, args=["qwerty", sublogger])

@melange396
Copy link
Collaborator Author

paired PR: cmu-delphi/covidcast-indicators#1885

Copy link
Contributor

@dmytrotsko dmytrotsko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, but seems to have formatting issues, everything above line 128 has 4 spaces/tab and everything below has 2 spaces

@melange396
Copy link
Collaborator Author

seems to have formatting issues, everything above line 128 has 4 spaces/tab and everything below has 2 spaces

old habits die hard... fixed that in the other PR and will copy it here shortly

dmytrotsko
dmytrotsko previously approved these changes Aug 9, 2023
Copy link
Contributor

@dmytrotsko dmytrotsko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dmytrotsko
dmytrotsko previously approved these changes Aug 14, 2023
Copy link
Contributor

@dmytrotsko dmytrotsko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@dmytrotsko dmytrotsko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@melange396 melange396 merged commit 01365a1 into dev Aug 14, 2023
@melange396 melange396 deleted the sync_loggers branch August 14, 2023 14:49
melange396 added a commit that referenced this pull request Aug 17, 2023
* Performance testing testing workflow (#1253)

* New CTIS publication (#1255)

* Newer CTIS publication

* bring loggers in sync and add multiproc capabilities (#1254)

* add syntax feature documentation (#1256)

* Newest CTIS publication

* chore: sync to www-covidcast release v3.2.7

* moving quidel signals to non-public access (#1261)

with integration tests!

---------

Co-authored-by: Dmytro Trotsko <[email protected]>

* chore: release delphi-epidata 4.1.8

---------

Co-authored-by: Dmytro Trotsko <[email protected]>
Co-authored-by: Rostyslav Zatserkovnyi <[email protected]>
Co-authored-by: Alex Reinhart <[email protected]>
Co-authored-by: nmdefries <[email protected]>
Co-authored-by: melange396 <[email protected]>
Co-authored-by: minhkhul <[email protected]>
Co-authored-by: minhkhul <[email protected]>
Co-authored-by: melange396 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants