-
Notifications
You must be signed in to change notification settings - Fork 7
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
Assembly.download module and Events.dump logging updated #203
Conversation
Example output when called with --verbose: 11/09/2023_02:07:40(PM) - INFO - Download the files |
parser.add_argument("-v", "--verbose", action="store_true", required=False, | ||
help="Verbose level logging") | ||
parser.add_argument("-d", "--debug", action="store_true", required=False, | ||
help="Debug level logging") |
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.
Maybe we should place these after the mandatory arguments ?
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.
Sure I can do, only reason I put them there was I thought it was needed given they are optional flag not positional args
"The add_argument() method must know whether an optional argument, like -f or --foo, or a positional argument, like a list of filenames, is expected. The first arguments passed to add_argument() must therefore be either a series of flags, or a simple argument 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 believe that sentence refers to the first arguments passed to the function add_argument()
, i.e. add_argument("-f", "--foo", ...)
, and not to the parser.
if args.verbose: | ||
logging.basicConfig( | ||
filename=log_file, format=logging_format, datefmt=date_format, | ||
filemode="w", level=logging.INFO | ||
) | ||
elif args.debug: | ||
logging.basicConfig( | ||
filename=log_file, format=logging_format, datefmt=date_format, | ||
filemode="w", level=logging.DEBUG | ||
) | ||
else: | ||
logging.basicConfig( | ||
filename=log_file, format=logging_format, datefmt=date_format, | ||
filemode="w", level=logging.WARNING | ||
) |
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 can factorize this further with something like:
if args.verbose: | |
logging.basicConfig( | |
filename=log_file, format=logging_format, datefmt=date_format, | |
filemode="w", level=logging.INFO | |
) | |
elif args.debug: | |
logging.basicConfig( | |
filename=log_file, format=logging_format, datefmt=date_format, | |
filemode="w", level=logging.DEBUG | |
) | |
else: | |
logging.basicConfig( | |
filename=log_file, format=logging_format, datefmt=date_format, | |
filemode="w", level=logging.WARNING | |
) | |
log_level = None | |
if args.debug: | |
log_level = logging.DEBUG | |
elif args.verbose: | |
log_level = logging.INFO | |
logging.basicConfig( | |
filename=log_file, format=logging_format, datefmt=date_format, | |
filemode="w", level=log_level | |
) |
Note that I've put debug before verbose: assuming we get the 2 params, I suppose debug should take priority.
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.
done
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.
This is actually better handled by the add_mutually_exclusive_group()
when adding the parameters to the parser [link].
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.
Basically just need to be rewritten/simplified with the new argparse and init_logging
self._connector.database = db_name | ||
try: | ||
self._connector.database = db_name | ||
except: | ||
logging.critical(f"Unknown database: '{db_name}' - Not located on host !") |
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 think this is needed: if there is an exception here, it should be allowed to be raised and the script should stop anyway
parser.add_argument("-v", "--verbose", action="store_true", required=False, | ||
help="Verbose level logging") | ||
parser.add_argument("-d", "--debug", action="store_true", required=False, | ||
help="Debug level logging") | ||
args = parser.parse_args() | ||
|
||
retrieve_assembly_data(**vars(args)) | ||
# Configure logging | ||
date_format='%Y/%m/%d_%I:%M:%S(%p)' | ||
logging_format='%(asctime)s - %(levelname)s - %(message)s' | ||
reload(logging) | ||
log_level = None | ||
if args.debug: | ||
log_level = logging.DEBUG | ||
elif args.verbose: | ||
log_level = logging.INFO | ||
|
||
logging.basicConfig( | ||
format=logging_format, datefmt=date_format, | ||
filemode="w", level=log_level | ||
) | ||
|
||
retrieve_assembly_data(args.accession, args.download_dir) |
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.
This should be rewritten with the new argparse and init_logging from ensembl-py
parser.add_argument("-v", "--verbose", action="store_true", required=False, | ||
help="Verbose level logging") | ||
parser.add_argument("-d", "--debug", action="store_true", required=False, | ||
help="Debug level logging") | ||
args = parser.parse_args() | ||
|
||
# Configure logging | ||
date_format='%Y/%m/%d_%I:%M:%S(%p)' | ||
logging_format='%(asctime)s - %(levelname)s - %(message)s' | ||
reload(logging) | ||
log_level = None | ||
if args.debug: | ||
log_level = logging.DEBUG | ||
elif args.verbose: | ||
log_level = logging.INFO | ||
|
||
logging.basicConfig( | ||
format=logging_format, datefmt=date_format, | ||
filemode="w", level=log_level | ||
) | ||
|
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.
Likewise, rewrite with new system
parser.add_argument("-v", "--verbose", action="store_true", required=False, | ||
help="Verbose level logging") | ||
parser.add_argument("-d", "--debug", action="store_true", required=False, | ||
help="Debug level logging") | ||
args = parser.parse_args() | ||
|
||
# Configure logging | ||
date_format='%Y/%m/%d_%I:%M:%S(%p)' | ||
logging_format='%(asctime)s - %(levelname)s - %(message)s' | ||
reload(logging) | ||
log_level = None | ||
if args.debug: | ||
log_level = logging.DEBUG | ||
elif args.verbose: | ||
log_level = logging.INFO | ||
|
||
logging.basicConfig( | ||
format=logging_format, datefmt=date_format, | ||
filemode="w", level=log_level | ||
) | ||
|
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.
Again, rewrite
Simple implementation of logging to file :
With log level dictated via command line args (flag).
If not defined default is set to WARNING, and file created with or without WARN level events being triggered.