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

Jump to bottom

Refactor configuration and results information #111

Merged
merged 4 commits into from
Nov 13, 2015
Merged

Refactor configuration and results information #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cfa8ca6
Refactoring serialised data into krun.results.Results. Simplified the…
Nov 12, 2015
c0c491f
Add __eq__ methods to basic classes. Used by unit tests. Add unit tes…
Nov 13, 2015
97c512c
Refactor configuration info into its own class. Audit is now a separa…
Nov 13, 2015
e89e9a9
Remove audits_same_platform. Audit objects can be compared directly.
Nov 13, 2015
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
195 changes: 87 additions & 108 deletions krun.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
import locale

import krun.util as util
from krun.config import Config
from krun.platform import detect_platform
from krun.results import Results
from krun.scheduler import ExecutionScheduler
from krun import ABS_TIME_FORMAT
from krun.mail import Mailer
Expand Down Expand Up @@ -40,21 +42,21 @@ def usage(parser):
def sanity_checks(config, platform):
vms_that_will_run = []
# check all necessary benchmark files exist
for bench, bench_param in config["BENCHMARKS"].items():
for vm_name, vm_info in config["VMS"].items():
for bench, bench_param in config.BENCHMARKS.items():
for vm_name, vm_info in config.VMS.items():
for variant in vm_info["variants"]:
entry_point = config["VARIANTS"][variant]
entry_point = config.VARIANTS[variant]
key = "%s:%s:%s" % (bench, vm_name, variant)
debug("Running sanity check for experiment %s" % key)

if util.should_skip(config, key):
if config.should_skip(key):
continue # won't execute, so no check needed

vm_info["vm_def"].check_benchmark_files(bench, entry_point)
vms_that_will_run.append(vm_name)

# per-VM sanity checks
for vm_name, vm_info in config["VMS"].items():
for vm_name, vm_info in config.VMS.items():
if vm_name not in vms_that_will_run:
# User's SKIP config directive may mean a defined VM never runs.
# This may be deliberate, e.g. the user does not yet have it built.
Expand Down Expand Up @@ -101,109 +103,90 @@ def sanity_check_user_change(platform):
def create_arg_parser():
"""Create a parser to process command-line options.
Copy link
Member

Choose a reason for hiding this comment

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

All of the changes to this block are quote substitutions? Which rules are you using to decide which to use?

Copy link
Member

Choose a reason for hiding this comment

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

Ah i see, you store a string of which dump.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I wrote the argument parsing code I used single quotes throughout, then I looked a bit further and realised that you had used double-quotes throughout. I thought I'd already changed all these to be consistent with your code, but apparently not.

Were you following some more complicated convention?

Copy link
Member

Choose a reason for hiding this comment

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

I don't currently have any rule in place. I actually had to search to see what the convention is.

There's no difference between ' and " in python, but it seems people use ' to express the notion of a string which is constant and " for format strings, which are conceptually expanded.

E.g.:

  • 'hello'
  • "hello %s" % name

We could adopt such a notion if you like(?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, up to you, but maybe we should change code as we come to it rather than a big bang. Let me know if you want me to change the strings in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Let's please not use arbitrary rules for "" and '' -- we'll never remember it, no-one will know why we did it, and so usage will quickly drift. Please just pick one and use it consistently in a file.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean Laurie. Do you mean use only " or ' in any given file?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. [And preferably be consistent in a project; but at least be consistent in a file.]

Copy link
Member

Choose a reason for hiding this comment

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

Right. We may find that we can't be this black and white, for example when nesting quotes. But we can try if you really feel strongly.

Not sure I want to impose any rules TBH.

"""
parser = argparse.ArgumentParser(description='Benchmark, running many fresh processes.')
parser = argparse.ArgumentParser(description="Benchmark, running many fresh processes.")

# Upper-case '-I' so as to make it harder to use by accident.
# Real users should never use -I. Only the OS init system.
parser.add_argument('--started-by-init', '-I', action='store_true',
default=False, dest='started_by_init', required=False,
help='Krun is being invoked by OS init system')
parser.add_argument('--resume', '-r', action='store_true', default=False,
dest='resume', required=False,
parser.add_argument("--started-by-init", "-I", action="store_true",
default=False, dest="started_by_init", required=False,
help="Krun is being invoked by OS init system")
parser.add_argument("--resume", "-r", action="store_true", default=False,
dest="resume", required=False,
help=("Resume benchmarking if interrupted " +
"and append to an existing results file"))
parser.add_argument('--reboot', '-b', action='store_true', default=False,
dest='reboot', required=False,
help='Reboot before each benchmark is executed')
parser.add_argument('--dryrun', '-d', action='store_true', default=False,
dest='dry_run', required=False,
parser.add_argument("--reboot", "-b", action="store_true", default=False,
dest="reboot", required=False,
help="Reboot before each benchmark is executed")
parser.add_argument("--dryrun", "-d", action="store_true", default=False,
dest="dry_run", required=False,
help=("Build and run a benchmarking schedule " +
"But don't execute the benchmarks. " +
"Useful for verifying configuration files"))
parser.add_argument('--debug', '-g', action="store", default='INFO',
dest='debug_level', required=False,
help=('Debug level used by logger. Must be one of: ' +
'DEBUG, INFO, WARN, DEBUG, CRITICAL, ERROR'))
parser.add_argument('--dump-audit', action="store_true",
dest='dump_audit', required=False,
help=('Print the audit section of a Krun ' +
'results file to STDOUT'))
parser.add_argument('--dump-config', action="store_true",
dest='dump_config', required=False,
help=('Print the config section of a Krun ' +
'results file to STDOUT'))
parser.add_argument('--dump-reboots', action="store_true",
dest='dump_reboots', required=False,
help=('Print the reboots section of a Krun ' +
'results file to STDOUT'))
parser.add_argument('--dump-etas', action="store_true",
dest='dump_etas', required=False,
help=('Print the eta_estimates section of a Krun ' +
'results file to STDOUT'))
parser.add_argument('--dump-temps', action="store_true",
dest='dump_temps', required=False,
help=('Print the starting_temperatures section of ' +
'a Krun results file to STDOUT'))
parser.add_argument('--dump-data', action="store_true",
dest='dump_data', required=False,
help=('Print the data section of ' +
'a Krun results file to STDOUT'))
parser.add_argument('--develop', action="store_true",
dest='develop', required=False,
help=('Enable developer mode'))
filename_help = ('Krun configuration or results file. FILENAME should' +
' be a configuration file when running benchmarks ' +
'(e.g. experiment.krun) and a results file ' +
'(e.g. experiment_results.json.bz2) when calling ' +
'krun with --dump-config, --dump_audit or ' +
'--dump-reboots')
parser.add_argument('filename', action="store", # Required by default.
metavar='FILENAME',
parser.add_argument("--debug", "-g", action="store", default='INFO',
dest="debug_level", required=False,
help=("Debug level used by logger. Must be one of: " +
"DEBUG, INFO, WARN, DEBUG, CRITICAL, ERROR"))
parser.add_argument("--dump-audit", action="store_const",
dest="dump", const="audit", required=False,
help=("Print the audit section of a Krun " +
"results file to STDOUT"))
parser.add_argument("--dump-config", action="store_const",
dest="dump", const="config", required=False,
help=("Print the config section of a Krun " +
"results file to STDOUT"))
parser.add_argument("--dump-reboots", action="store_const",
dest="dump", const="reboots", required=False,
help=("Print the reboots section of a Krun " +
"results file to STDOUT"))
parser.add_argument("--dump-etas", action="store_const",
dest="dump", const="eta_estimates", required=False,
help=("Print the eta_estimates section of a Krun " +
"results file to STDOUT"))
parser.add_argument("--dump-temps", action="store_const",
dest="dump", const="starting_temperatures",
required=False,
help=("Print the starting_temperatures section of " +
"a Krun results file to STDOUT"))
parser.add_argument("--dump-data", action="store_const",
dest="dump", const="data", required=False,
help=("Print the data section of " +
"a Krun results file to STDOUT"))
parser.add_argument("--develop", action="store_true",
dest="develop", required=False,
help=("Enable developer mode"))
filename_help = ("Krun configuration or results file. FILENAME should" +
" be a configuration file when running benchmarks " +
"(e.g. experiment.krun) and a results file " +
"(e.g. experiment_results.json.bz2) when calling " +
"krun with --dump-config, --dump_audit, " +
"--dump-reboots, --dump-etas, --dump-temps, or"
"--dump-data")
parser.add_argument("filename", action="store", # Required by default.
metavar="FILENAME",
help=(filename_help))
return parser

def dump_section(args):
results = util.read_results(args.filename)
if args.dump_config:
text = results['config']
elif args.dump_audit:
text = util.dump_audit(results['audit'])
elif args.dump_reboots:
text = str(results['reboots'])
elif args.dump_etas:
text = json.dumps(results['eta_estimates'],
sort_keys=True, indent=2)
elif args.dump_temps:
text = json.dumps(results['starting_temperatures'],
sort_keys=True, indent=2)
elif args.dump_data:
text = json.dumps(results['data'],
sort_keys=True, indent=2)
else:
assert False # unreachable

# String data read in from JSON are unicode objects. This matters for us
# as some data in the audit includes unicode characters. If it does,
# a simple print no longer suffices if the system locale is (e.g.) ASCII.
# In this case print will raise.
#
# The correct thing to do is to encode() the unicode to the system locale.
print(text.encode(locale.getpreferredencoding()))


def main(parser):
args = parser.parse_args()

if (args.dump_config or
args.dump_audit or
args.dump_reboots or
args.dump_etas or
args.dump_temps or
args.dump_data):
if args.dump is not None:
if not args.filename.endswith(".json.bz2"):
usage(parser)
else:
dump_section(args)
results = Results(None, None, results_file=args.filename)
if args.dump == "config" or "audit":
text = unicode(results.__getattribute__(args.dump))
else:
text = json.dumps(results.__getattribute__(args.dump),
sort_keys=True, indent=2)
# String data read in from JSON are unicode objects. This matters
# for us as some data in the audit includes unicode characters.
# If it does, a simple print no longer suffices if the system
# locale is (e.g.) ASCII. In this case print will raise an
# exception. The correct thing to do is to encode() the unicode to
# the system locale.
print(text.encode(locale.getpreferredencoding()))
sys.exit(0)

if not args.filename.endswith(".krun"):
Expand All @@ -215,8 +198,8 @@ def main(parser):
except OSError:
util.fatal('Krun configuration file %s does not exist.' % args.filename)

config = util.read_config(args.filename)
out_file = util.output_name(args.filename)
config = Config(args.filename)
out_file = config.results_filename()
out_file_exists = os.path.exists(out_file)

if out_file_exists and not os.path.isfile(out_file):
Expand All @@ -240,12 +223,11 @@ def main(parser):
if args.develop:
warn("Developer mode enabled. Results will not be reliable.")

mail_recipients = config.get("MAIL_TO", [])
mail_recipients = config.MAIL_TO
if type(mail_recipients) is not list:
util.fatal("MAIL_TO config should be a list")

max_mails = config.get("MAX_MAILS", 5)
mailer = Mailer(mail_recipients, max_mails=max_mails)
mailer = Mailer(mail_recipients, max_mails=config.MAX_MAILS)

# Initialise platform instance and assign to VM defs.
# This needs to be done early, so VM sanity checks can run.
Expand All @@ -260,8 +242,6 @@ def main(parser):
platform.developer_mode = True

platform.collect_audit()
for vm_name, vm_info in config["VMS"].items():
vm_info["vm_def"].set_platform(platform)

# If the user has asked for resume-mode, the current platform must
# be an identical machine to the current one.
Expand All @@ -273,12 +253,13 @@ def main(parser):
if args.resume:
# output file must exist, due to check above
assert(out_file_exists)
current = util.read_results(out_file)
if not util.audits_same_platform(platform.audit, current["audit"]):
current = Results(config, platform, results_file=out_file)
from krun.audit import Audit
if not Audit(platform.audit) == current.audit:
util.fatal(error_msg)

debug("Using pre-recorded initial temperature readings")
platform.set_starting_temperatures(current["starting_temperatures"])
platform.set_starting_temperatures(current.starting_temperatures)
else:
Copy link
Member

Choose a reason for hiding this comment

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

Note to self, figure out how platform is set in the VM def

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, where does that happen actually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

krun.scheduler.Scheduler lines 131--133 in the constructor.

Copy link
Member

Choose a reason for hiding this comment

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

OK

# Touch the config file to update its mtime. This is required
# by resume-mode which uses the mtime to determine the name of
Expand All @@ -290,21 +271,19 @@ def main(parser):
debug("Taking fresh initial temperature readings")
platform.set_starting_temperatures()

log_filename = attach_log_file(args.filename, args.resume)
attach_log_file(config, args.resume)

sanity_checks(config, platform)

# Build job queue -- each job is an execution
sched = ExecutionScheduler(args.filename,
log_filename,
out_file,
sched = ExecutionScheduler(config,
mailer,
platform,
resume=args.resume,
reboot=args.reboot,
dry_run=args.dry_run,
started_by_init=args.started_by_init)
sched.build_schedule(config, current)
sched.build_schedule()

# does the benchmarking
sched.run()
Expand All @@ -331,13 +310,13 @@ def setup_logging(parser):
logging.root.handlers = [stream]


def attach_log_file(config_filename, resume):
log_filename = util.log_name(config_filename, resume)
def attach_log_file(config, resume):
log_filename = config.log_filename(resume)
mode = 'a' if resume else 'w'
fh = logging.FileHandler(log_filename, mode=mode)
fh.setFormatter(PLAIN_FORMATTER)
logging.root.addHandler(fh)
return os.path.abspath(log_filename)
return


if __name__ == "__main__":
Expand Down
6 changes: 6 additions & 0 deletions krun/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
UNKNOWN_ABS_TIME = "????-??-?? ??:??:??"

class EntryPoint(object):

def __init__(self, target, subdir=None):
self.target = target
self.subdir = subdir

def __eq__(self, other):
return (isinstance(other, self.__class__) and
(self.target == other.target) and
(self.subdir == other.subdir))
50 changes: 50 additions & 0 deletions krun/audit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
from collections import OrderedDict
from logging import debug


class Audit(object):

def __init__(self, audit_dict):
assert isinstance(audit_dict, dict)
self._audit = audit_dict

def __contains__(self, key):
return key in self._audit

def __getitem__(self, key):
return self._audit[key]

def __setitem__(self, key, value):
self._audit[key] = value

def __unicode__(self):
s = ""
# important that the sections are sorted, for diffing
for key, text in OrderedDict(sorted(self._audit.iteritems())).iteritems():
s += "Audit Section: %s" % key + "\n"
s += "#" * 78 + "\n\n"
s += unicode(text) + "\n\n"
return s

def __len__(self):
return len(self._audit)

@property
def audit(self):
return self._audit

@audit.setter
def audit(self, audit_dict):
self._audit = audit_dict

def __eq__(self, other):
if ((not isinstance(other, self.__class__)) or
(not len(self) == len(other))):
return False
for key in self._audit:
if key == "packages" and (key in self) and (key in other):
continue
if ((key not in other._audit) or (other[key] != self[key])):
debug("%s keys differed in audit" % key)
return False
return True
Loading