Skip to content
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.

Sign up for GitHub

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

Feature/logging #85

Merged
merged 11 commits into from
Aug 29, 2017
Merged

Feature/logging #85

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c01055b
added log method in class Generator for storing information about com…
Aug 25, 2017
0bbb959
added loging current working directory
Aug 26, 2017
53c15d4
fixed pep8 issues without changing some lines
Aug 26, 2017
f2a2fe4
merged save_logs and log methods and minor esthetical changes
Aug 26, 2017
0d5b989
changed logging using python logging feature
Aug 28, 2017
cc7b83c
some fixes in logging
Aug 28, 2017
7706566
changed imports order in generator.py according to imports formating …
Aug 28, 2017
d9fb0ab
changed logger naming
Aug 29, 2017
dc2ead7
changed logger naming
Aug 29, 2017
11c3a80
Merge branch 'feature/logging' of https://github.com/PanSzczerba/mcpa…
Aug 29, 2017
38ff22e
added some formatting for logging batch system output
Aug 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions mcpartools/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import os
import getpass
import logging
import os
import shutil
import socket
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please sort imports.
https://google.github.io/styleguide/pyguide.html#Imports_formatting

Within each grouping, imports should be sorted lexicographically, ignoring case, according to each module's full package path.

import sys
import time

from mcpartools.mcengine.common import EngineDiscover
from mcpartools.scheduler.common import SchedulerDiscover

logger = logging.getLogger(__name__)

file_logger = logging.getLogger('file_logger')
file_logger.setLevel(logging.INFO)
file_logger.propagate = False


class Options:

Expand Down Expand Up @@ -139,7 +146,7 @@ def run(self):
# make symlinks to external files found
self.symlink_external_files()

# save logs
# store information about command line arguments, date, time, user and hostname into generatemc.log
self.save_logs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see the point of having self.log followed by self.save_logs. If both aim at doing the same then why not merging them ? If they do sth else, then write better comments.

Do not hesitate to question existing architecture because then you fall in trap like "I did it in log cause I didn't know what save_log was meant to do"


return 0
Expand All @@ -159,6 +166,8 @@ def generate_main_dir(self):
os.mkdir(dir_path)
self.main_dir = dir_path

file_logger.addHandler(logging.FileHandler(os.path.join(dir_path, "generatemc.log"), mode='w+'))

def generate_workspace(self):
wspdir_name = 'workspace'
wspdir_path = os.path.join(self.main_dir, wspdir_name)
Expand Down Expand Up @@ -225,4 +234,7 @@ def symlink_external_files(self):
os.symlink(abs_path, os.path.join(jobdir_path, os.path.split(abs_path)[-1]))

def save_logs(self):
pass
file_logger.info('Executed command: ' + ' '.join(sys.argv))
file_logger.info('Date and time: ' + time.strftime("%Y-%m-%d %H:%M:%S"))
file_logger.info('username@hostname: ' + getpass.getuser() + '@' + socket.gethostname())
file_logger.info('Current working directory: ' + os.getcwd())
29 changes: 14 additions & 15 deletions mcpartools/scheduler/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import logging
import os
from subprocess import check_call, CalledProcessError
from subprocess import check_output, CalledProcessError

from mcpartools.scheduler.slurm import Slurm
from mcpartools.scheduler.torque import Torque
Expand All @@ -16,17 +15,17 @@ def __init__(self):

@classmethod
def get_scheduler(cls, scheduler_options, log_location):
with open(os.path.join(log_location, "generatemc.log"), 'w+') as LOG_FILE:
try:
check_call(['srun --version'], stdout=LOG_FILE, stderr=LOG_FILE, shell=True)
logger.debug("Discovered job scheduler SLURM")
return Slurm(scheduler_options)
except CalledProcessError as e:
logger.debug("Slurm not found: %s", e)
try:
check_call(['qsub --version'], stdout=LOG_FILE, stderr=LOG_FILE, shell=True)
logger.debug("Discovered job scheduler Torque")
return Torque(scheduler_options)
except CalledProcessError as e:
logger.debug("Torque not found: %s", e)
file_logger = logging.getLogger('file_logger')
try:
file_logger.info(check_output(['srun --version'], shell=True))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I am correct check_output returns output of the command.
Please assign it first to the some variable, i.e. and log it with some description:

srun_output = check_output(...)
file_logger.info("srun info {:s}".format(...))
``

logger.debug("Discovered job scheduler SLURM")
Copy link
Contributor

@antnieszka antnieszka Aug 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If it's debug logging level - I would add slurm/torque version to this log too. Same in line 29.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is already saved to the file (but not printed on screen), even on debug level:

# plgkongruencj at p1728 in /net/scratch/people/plgkongruencj/mcpartools on git:feature/logging ✖︎ [11:44:29]
→ cat aa/run_20170829_114429/generatemc.log 
srun output: slurm 17.02.7
Executed command: mcpartools/generatemc.py -j 10 -p 100 tests/res/shieldhit -w aa -vvv
Date and time: 2017-08-29 11:44:29
username@hostname: plgkongruencj@p1728
Current working directory: /net/scratch/people/plgkongruencj/mcpartools

return Slurm(scheduler_options)
except CalledProcessError as e:
logger.debug("Slurm not found: %s", e)
try:
file_logger.info(check_output(['qsub --version'], shell=True))
logger.debug("Discovered job scheduler Torque")
return Torque(scheduler_options)
except CalledProcessError as e:
logger.debug("Torque not found: %s", e)
raise SystemError("No known batch system found!")