Feature/logging #85

PanSzczerba · 2017-08-26T10:05:46Z

Added new log method in generator.py implementing features from "better logging" issue

grzanka · 2017-08-26T10:32:34Z

@PanSzczerba why do I see my commits in the list https://github.com/DataMedSci/mcpartools/pull/85/commits ?

Is your fork and your master branch up-to-date with https://github.com/DataMedSci/mcpartools ?

grzanka · 2017-08-26T10:33:12Z

Fix PEP8 issues https://travis-ci.org/DataMedSci/mcpartools/jobs/268642089

codecov-io · 2017-08-26T11:11:21Z

Codecov Report

Merging #85 into master will increase coverage by 0.34%.
The diff coverage is 46.42%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   63.98%   64.32%   +0.34%     
==========================================
  Files          12       12              
  Lines         844      855      +11     
==========================================
+ Hits          540      550      +10     
- Misses        304      305       +1

Impacted Files	Coverage Δ
mcpartools/generator.py	`73.49% <100%> (+1.69%)`	⬆️
mcpartools/scheduler/common.py	`34.61% <6.25%> (-5.39%)`	⬇️
mcpartools/_version.py	`43.32% <0%> (+0.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e02ebcd...38ff22e. Read the comment docs.

grzanka

Before continuing, please ensure that you did the PR correctly. First two commits in this PR shouldn't be here.

grzanka · 2017-08-26T11:13:38Z

mcpartools/generator.py

@@ -111,6 +114,9 @@ def run(self):
        # make symlinks to external files found
        self.symlink_external_files()

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


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"

grzanka · 2017-08-26T11:13:58Z

mcpartools/generator.py

@@ -190,3 +196,13 @@ def symlink_external_files(self):

    def save_logs(self):
        pass
+
+    def log(self):
+        with open(os.path.join(self.main_dir, "generatemc.log"), 'a') as LOG_FILE:


Why LOG_FILE is capitalized ?

Ok so should I create a new pull request without them?

It depends on your git knowledge. Your branch history is broken, see:
https://github.com/PanSzczerba/mcpartools/commits/feature/logging

Why commits b8c0b28 and f480569 are there under 26 of August ?

You can edit your commits history and do push --force or do new branch and new PR.

…mand line arguments, date, time, user and hostname in generatemc.log

PanSzczerba · 2017-08-26T14:46:37Z

Ok, so I think it should be fine right now. I have fixed commit history and did some minor changes You asked

grzanka · 2017-08-26T16:51:09Z

mcpartools/generator.py

+            log_file.write("\n")
+            log_file.write(time.strftime("%Y-%m-%d %H:%M:%S\n"))
+            log_file.write(getpass.getuser() + '@' + socket.gethostname() + "\n")
+            log_file.write(os.getcwd() + "\n")


Add some description what is being logged, like:

Command executed: Date & time: User@hostname: Current directory:

grzanka · 2017-08-26T16:54:01Z

mcpartools/generator.py

@@ -225,4 +228,10 @@ 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
+        with open(os.path.join(self.main_dir, "generatemc.log"), 'a') as log_file:


filename "generatemc.log" occurs now twice, see also here:

mcpartools/mcpartools/scheduler/common.py

Line 19 in 35d92ad

with open(os.path.join(log_location, "generatemc.log"), 'w+') as LOG_FILE:

with open(os.path.join(log_location, "generatemc.log"), 'w+') as LOG_FILE: try:

What about unifying this ?

Can it be done in some generic way ?
Would it be possible to use python logging to write to such a file in some standarized way ?

See logging handlers https://docs.python.org/3.5/library/logging.html

PanSzczerba · 2017-08-28T19:55:22Z

Ok so I tried using logging module. But I had to create another logger with it's own handler and I don't know if that's a good idea.

antnieszka · 2017-08-28T19:59:05Z

Maybe this could help:
https://github.com/DataMedSci/ProtonBeamsComposer/blob/master/pbc/optimize.py#L95
(I simply add a handle to global logger and it works just fine)

grzanka · 2017-08-28T20:10:49Z

mcpartools/generator.py

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

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


why "generatemc.log" occurs here and also in 'getLogger' method ?

I wanted to add FileHandler directly after the main_dir path is assigned so it could log information about batch system in scheduler/common.py. And also I wanted to name this logger to be unique and so I could acces it in scheduler/common.py but I couldn't come up with a better idea how to name it.

Well, it could be named file_logger (the variable) and the logger name also file_logger

grzanka · 2017-08-28T20:13:14Z

mcpartools/generator.py

@@ -2,12 +2,19 @@
 import logging
 import shutil
 import time
+import sys
+import getpass
+import socket


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.

…guidelines

…rtools into feature/logging Conflicts: mcpartools/scheduler/common.py

grzanka · 2017-08-29T08:41:07Z

mcpartools/scheduler/common.py

-                logger.debug("Torque not found: %s", e)
+        file_logger = logging.getLogger('file_logger')
+        try:
+            file_logger.info(check_output(['srun --version'], shell=True))


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(...)) ``

antnieszka · 2017-08-29T09:26:31Z

mcpartools/scheduler/common.py

@@ -17,13 +17,15 @@ def __init__(self):
    def get_scheduler(cls, scheduler_options, log_location):
        file_logger = logging.getLogger('file_logger')
        try:
-            file_logger.info(check_output(['srun --version'], shell=True))
+            srun_output = check_output(['srun --version'], shell=True)
+            file_logger.info("srun output: {}".format(srun_output[:-1]))
            logger.debug("Discovered job scheduler SLURM")


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

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

grzanka

I approve this PR as it is
The issue is however not closed, there are more tasks to be done, which I will define on issue level.

grzanka added the in progress label Aug 26, 2017

grzanka requested changes Aug 26, 2017

View reviewed changes

PanSzczerba force-pushed the feature/logging branch from 7e25a1b to 75ae913 Compare August 26, 2017 11:54

Michał and others added 3 commits August 26, 2017 16:22

added log method in class Generator for storing information about com…

c01055b

…mand line arguments, date, time, user and hostname in generatemc.log

added loging current working directory

0bbb959

fixed pep8 issues without changing some lines

53c15d4

PanSzczerba force-pushed the feature/logging branch from 75ae913 to 53c15d4 Compare August 26, 2017 14:24

merged save_logs and log methods and minor esthetical changes

f2a2fe4

grzanka reviewed Aug 26, 2017

View reviewed changes

Michal Szczerba added 2 commits August 28, 2017 18:56

changed logging using python logging feature

0d5b989

some fixes in logging

cc7b83c

antnieszka added this to the Next release milestone Aug 28, 2017

antnieszka mentioned this pull request Aug 28, 2017

Better logging #84

Open

grzanka reviewed Aug 28, 2017

View reviewed changes

Michal Szczerba added 4 commits August 28, 2017 22:36

changed imports order in generator.py according to imports formating …

7706566

…guidelines

changed logger naming

d9fb0ab

changed logger naming

dc2ead7

Merge branch 'feature/logging' of https://github.com/PanSzczerba/mcpa…

11c3a80

…rtools into feature/logging Conflicts: mcpartools/scheduler/common.py

grzanka reviewed Aug 29, 2017

View reviewed changes

added some formatting for logging batch system output

38ff22e

antnieszka reviewed Aug 29, 2017

View reviewed changes

grzanka approved these changes Aug 29, 2017

View reviewed changes

grzanka added Type: feature and removed in progress labels Aug 29, 2017

grzanka merged commit 4c8bc3c into DataMedSci:master Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/logging #85

Feature/logging #85

PanSzczerba commented Aug 26, 2017

grzanka commented Aug 26, 2017

grzanka commented Aug 26, 2017

codecov-io commented Aug 26, 2017 •

edited

Loading

grzanka left a comment

grzanka Aug 26, 2017

grzanka Aug 26, 2017

PanSzczerba Aug 26, 2017

grzanka Aug 26, 2017

PanSzczerba commented Aug 26, 2017

grzanka Aug 26, 2017

grzanka Aug 26, 2017

PanSzczerba commented Aug 28, 2017

antnieszka commented Aug 28, 2017 •

edited

Loading

grzanka Aug 28, 2017

PanSzczerba Aug 28, 2017

grzanka Aug 28, 2017

grzanka Aug 28, 2017

grzanka Aug 29, 2017

antnieszka Aug 29, 2017 •

edited

Loading

grzanka Aug 29, 2017

grzanka left a comment

Feature/logging #85

Feature/logging #85

Conversation

PanSzczerba commented Aug 26, 2017

grzanka commented Aug 26, 2017

grzanka commented Aug 26, 2017

codecov-io commented Aug 26, 2017 • edited Loading

Codecov Report

grzanka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanSzczerba commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanSzczerba commented Aug 28, 2017

antnieszka commented Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antnieszka Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzanka left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 26, 2017 •

edited

Loading

antnieszka commented Aug 28, 2017 •

edited

Loading

antnieszka Aug 29, 2017 •

edited

Loading