Conversation
WMDConductor
left a comment
There was a problem hiding this comment.
LGTM - with small exception catching nit + Codacy considerations assumed sorted out.
conductor/lib/common.py
Outdated
There was a problem hiding this comment.
Seems like we could make this: catch explicit:
except (ValueError, TypeError):
| import multiprocessing | ||
| import os | ||
| import platform | ||
| from pprint import pformat |
There was a problem hiding this comment.
from x import y go after import x statements.
eg. this is what isort gives me:
import base64
import datetime
import functools
import hashlib
import json
import logging
import multiprocessing
import os
import platform
import random
import signal
import subprocess
import sys
import time
import traceback
from pprint import pformat
import yaml
There was a problem hiding this comment.
We should probably decide as a team on how we want to sort imports. I haven't seen much in google style guide or pep8 (aside from grouping imports by builtin, thirdparty, internal).
Personally, I sort imports alphabetically, considering the uppermost namespace (parent packages) when evaluating order. I don't distinguish/consider whether it's a from vs import. I find that it makes it easier for me to look something up, if there is one continuous alphabetical ordering, rather than having multiple sections of their own ordering. But we're all weird humans, thinking in weird ways.
also, I noticed that isort has several different options that make dramatic differences (--no-sections, --order-by-type, --project, --thirdparty , etc). Might be worth playing around with these to find a workflow something that we all like (dislike ;) ).
There was a problem hiding this comment.
Good call, let's make this a team decision.
conductor/lib/common.py
Outdated
There was a problem hiding this comment.
That was the intended spelling. But after reading this: https://english.stackexchange.com/questions/122627/trawling-through-or-trolling-through
....I'm wondering if I should just change it to your recommendation (English is so hard!)
| try: | ||
| logger.info(self.manager.worker_queue_status_text()) | ||
| logger.info(self.upload_status_text()) | ||
| except Exception: |
There was a problem hiding this comment.
except: would also work as a catch-all.
There was a problem hiding this comment.
I've been going back and forth on this recently (due to recent revelations that I'm somewhat embarrassed about). And coincidentally, this module makes for the perfect example!
As much as I have loved to catch all exceptions by simply not listing any specific types (when deemed appropriate), I didn't realize there was a subtle difference in doing so vs explicitly catching the Exception class. See https://stackoverflow.com/questions/730764/how-to-properly-ignore-exceptions
Usually it doesn't matter if we "catch all" exception, but because this module's application (the uploader), is a long running (daemon) process, that can only be stopped via killing it (ctrl-c, sigint, sigterm, etc), we have to be thoughtful to not disrupt any logic in the uploader that is expecting to encounter ctrl-c (KeyboardInterrupt).
Here's a great example of how you can unintentionally make trouble : https://fman.io/blog/an-inescapable-while-loop-in-python/
side note: here's an interesting diagram/hierarchy of pythons exception classes (as well as general good reading): https://airbrake.io/blog/python-exception-handling/class-hierarchy
conductor/lib/worker.py
Outdated
There was a problem hiding this comment.
Can you rename the method to kill_metric_store for consistency with the above variable self.metric_store?
There was a problem hiding this comment.
sure, makes sense
| return (queue.get_nowait()) | ||
| except Queue.Empty: | ||
| return | ||
| except Exception: |
There was a problem hiding this comment.
This can be simplified to except:.
There was a problem hiding this comment.
857329c to
fc0766e
Compare
fc0766e to
4a1f334
Compare
| import multiprocessing | ||
| import os | ||
| import platform | ||
| from pprint import pformat |
There was a problem hiding this comment.
Good call, let's make this a team decision.
- Now properly terminates the following threads after uploading a job: ErrorThread MetricStore PrintStatusThread ReporterThread - Disclaimer: this code really stinks. Needs an entire re-write.
- Activated for POST /jobs/
4a1f334 to
03a1f1d
Compare
No description provided.