Skip to content
Open
Changes from all commits
Commits
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
198 changes: 90 additions & 108 deletions contrib/vcloud/benchmarkclient_executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import sys
from pathlib import Path

import yaml

import benchexec.util
from benchexec import BenchExecException
from benchexec.tooladapter import CURRENT_BASETOOL, create_tool_locator
Expand All @@ -23,7 +25,6 @@

DEFAULT_CLOUD_MEMORY_REQUIREMENT = 7_000_000_000 # 7 GB
DEFAULT_CLOUD_CPUCORE_REQUIREMENT = 2 # one core with hyperthreading
DEFAULT_CLOUD_CPUMODEL_REQUIREMENT = "" # empty string matches every model

STOPPED_BY_INTERRUPT = False

Expand Down Expand Up @@ -63,7 +64,7 @@ def init(config, benchmark):
raise BenchExecException(
f"Executable path {executable_for_version} is not relative"
" and is not containing the expected mount point in the container"
" {TOOL_DIRECTORY_MOUNT_POINT}."
f" {TOOL_DIRECTORY_MOUNT_POINT}."
) from e

# ensure that executable_for_version is not pointing to a directory
Expand Down Expand Up @@ -114,7 +115,7 @@ def execute_benchmark(benchmark, output_handler):
# build input for cloud
(cloudInput, numberOfRuns) = getCloudInput(benchmark)
if benchmark.config.debug:
cloudInputFile = os.path.join(benchmark.log_folder, "cloudInput.txt")
cloudInputFile = os.path.join(benchmark.log_folder, "cloudInput.yml")
benchexec.util.write_file(cloudInput, cloudInputFile)
output_handler.all_created_files.add(cloudInputFile)
meta_information = json.dumps(
Expand Down Expand Up @@ -168,6 +169,8 @@ def execute_benchmark(benchmark, output_handler):
cmdLine.extend(["--print-new-files", "true"])
if benchmark.config.containerImage:
cmdLine.extend(["--containerImage", str(benchmark.config.containerImage)])
cmdLine.extend(["--input-format", "yaml"])
cmdLine.extend(["--output-dir", benchmark.log_folder])

start_time = benchexec.util.read_local_time()

Expand Down Expand Up @@ -212,122 +215,63 @@ def formatEnvironment(environment):
return ";".join(f"{k}={v}" for k, v in environment.get("newEnv", {}).items())


def toTabList(items):
return "\t".join(map(str, items))


def getCloudInput(benchmark):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need a link here that points to a place where the format for the cloud-input file is described.

(
requirements,
numberOfRuns,
limitsAndNumRuns,
runDefinitions,
sourceFiles,
) = getBenchmarkDataForCloud(benchmark)
(workingDir, toolpaths) = getToolDataForCloud(benchmark)

# prepare cloud input, we make all paths absolute, TODO necessary?
outputDir = benchmark.log_folder
absOutputDir = os.path.abspath(outputDir)
absWorkingDir = os.path.abspath(workingDir)
absToolpaths = list(map(os.path.abspath, toolpaths))
absSourceFiles = list(map(os.path.abspath, sourceFiles))
absBaseDir = benchexec.util.common_base_dir(absSourceFiles + absToolpaths)

if absBaseDir == "":
sys.exit("No common base dir found.")

numOfRunDefLinesAndPriorityStr = [numberOfRuns + 1] # add 1 for the headerline
if benchmark.config.cloudPriority:
numOfRunDefLinesAndPriorityStr.append(benchmark.config.cloudPriority)

# build the input for the cloud,
# see external vcloud/README.txt for details.
cloudInput = [
toTabList(absToolpaths),
toTabList([absBaseDir, absOutputDir, absWorkingDir]),
toTabList(requirements),
]
if benchmark.result_files_patterns:
if len(benchmark.result_files_patterns) > 1:
sys.exit("Multiple result-files patterns not supported in cloud mode.")
cloudInput.append(benchmark.result_files_patterns[0])

cloudInput.extend(
[toTabList(numOfRunDefLinesAndPriorityStr), toTabList(limitsAndNumRuns)]
)
cloudInput.extend(runDefinitions)
return ("\n".join(cloudInput), numberOfRuns)
absBaseDir, numberOfRuns = computeBaseDir(benchmark, absToolpaths)


def getBenchmarkDataForCloud(benchmark):
# get requirements
r = benchmark.requirements
memRequirement = bytes_to_mb(
DEFAULT_CLOUD_MEMORY_REQUIREMENT if r.memory is None else r.memory
)
requirements = [
memRequirement,
DEFAULT_CLOUD_CPUCORE_REQUIREMENT if r.cpu_cores is None else r.cpu_cores,
DEFAULT_CLOUD_CPUMODEL_REQUIREMENT if r.cpu_model is None else r.cpu_model,
]

# get limits and number of Runs
timeLimit = benchmark.rlimits.cputime_hard
memLimit = bytes_to_mb(benchmark.rlimits.memory) or memRequirement
coreLimit = benchmark.rlimits.cpu_cores
wallTimeLimit = benchmark.rlimits.walltime
numberOfRuns = sum(
len(runSet.runs) for runSet in benchmark.run_sets if runSet.should_be_executed()
)
limitsAndNumRuns = [numberOfRuns, timeLimit, memLimit]
if coreLimit is not None:
limitsAndNumRuns.append(coreLimit)
else:
limitsAndNumRuns.append("-")
if wallTimeLimit is not None:
# WallTimeLimit has to be the 5th element of limitsAndNumRuns
assert len(limitsAndNumRuns) == 4
limitsAndNumRuns.append(wallTimeLimit)
else:
limitsAndNumRuns.append("-")

# get Runs with args and sourcefiles
sourceFiles = []
runDefinitions = []
for runSet in benchmark.run_sets:
if not runSet.should_be_executed():
continue
if STOPPED_BY_INTERRUPT:
break
# get limits
rlimits = benchmark.rlimits
timeLimit = int(rlimits.cputime_hard)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Limits should be ints anyway. But even if we encounter a limit that is not an int, we would not want to silently truncate the value and use a wrong value! So please remove all the int() calls.


# get runs
for run in runSet.runs:
cmdline = run.cmdline()
cmdline = list(map(vcloudutil.force_linux_path, cmdline))
limits_cpu = {"time": {"hard": timeLimit}}
if rlimits.cpu_cores is not None:
limits_cpu["cores"] = rlimits.cpu_cores

# we assume, that VCloud-client only splits its input at tabs,
# so we can use all other chars for the info, that is needed to run the tool.
argString = json.dumps(cmdline)
assert "\t" not in argString # cannot call toTabList(), if there is a tab
limits = {"cpu": limits_cpu}
if rlimits.walltime is not None:
limits["walltime"] = int(rlimits.walltime)
Comment on lines +233 to +239

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it really correct that the time limits look like this in the yaml?

limits:
  cpu:
    time:
      hard: 1
  walltime: 2

Looks quite inconsistent.

if rlimits.memory is not None:
limits["memory"] = rlimits.memory

log_file = os.path.relpath(run.log_file, benchmark.log_folder)
if os.path.exists(run.identifier):
runDefinitions.append(
toTabList(
[argString, log_file] + run.sourcefiles + run.required_files
)
)
else:
runDefinitions.append(
toTabList([argString, log_file] + run.required_files)
)
sourceFiles.extend(run.sourcefiles)
runDefinitions = buildRunDefinitions(benchmark, limits, absBaseDir)

if not runDefinitions:
sys.exit("Benchmark has nothing to run.")

return (requirements, numberOfRuns, limitsAndNumRuns, runDefinitions, sourceFiles)
cloud_input = {
"formatVersion": "1.0",
"files": [os.path.relpath(p, absBaseDir) for p in absToolpaths],
"basedir": os.path.relpath(absBaseDir),
"execdir": os.path.relpath(absWorkingDir, absBaseDir),
}

if benchmark.config.cloudPriority:
cloud_input["priority"] = benchmark.config.cloudPriority

cloud_input["memoryreq"] = (
r.memory if r.memory is not None else DEFAULT_CLOUD_MEMORY_REQUIREMENT
)
cloud_input["corereq"] = (
r.cpu_cores if r.cpu_cores is not None else DEFAULT_CLOUD_CPUCORE_REQUIREMENT
)
if r.cpu_model:
cloud_input["cpumodels"] = [r.cpu_model]
Comment on lines +258 to +265

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find it inconsistent that limits are specified in some nested yaml structure, but requirements are flattened into top-level keys, some of which have the name suffix req and one of which doesn't.

Furthermore, core limits are given as limits: cpu: cores: and core requirements are given as corereq:, which is inconsistent both with respect to plural/singular and with respect to whether "CPU" is an explicit part of the naming.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is cpumodels a list and what are the semantics if there is more than one entry?

Also note that r.cpu_model can already be a list of CPU models (e.g., 'i9-9900, i7-6700'), so we can end up with a list of list of CPU models, which could be confusing and error prone.


if benchmark.result_files_patterns:
cloud_input["resultFilePatterns"] = list(benchmark.result_files_patterns)
Comment on lines +267 to +268

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe here it should be explained what the special case for relative patterns is compared to the other relative paths.

And doesn't the BenchCloud forbid absolute patterns? Is this handled somewhere else?


cloud_input["runsets"] = runDefinitions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the BenchCloud distinguish between different run sets in a single submission?

AFAIU there is just a list of runs per run collection, and all have the same settings such as limits. So it seems redundant to split the input into different run sets and submit the limits for each of them.

Or is the intent to change this? Then we should discuss how to best map the BenchExec structures to what the BenchCloud wants to support.


return yaml.dump(
cloud_input, default_flow_style=False, allow_unicode=True
), numberOfRuns


def getToolDataForCloud(benchmark):
Expand Down Expand Up @@ -358,6 +302,50 @@ def getToolDataForCloud(benchmark):
return (workingDir, validToolpaths)


def computeBaseDir(benchmark, absToolpaths):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't this need to take all files into account that are part of the input, e.g., also the required files?

absSourceFiles = []
numberOfRuns = 0
for runSet in activeRunSets(benchmark):
for run in runSet.runs:
if os.path.exists(run.identifier):
absSourceFiles.extend(map(os.path.abspath, run.sourcefiles))
numberOfRuns += 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would say the calculation of numberofRuns should be removed from this method. It is not really related to computeBaseDir, and trivial especially using an expression like sum(len(runSet.runs) for ...).


absBaseDir = benchexec.util.common_base_dir(absSourceFiles + absToolpaths)
if absBaseDir == "":
sys.exit("No common base dir found.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a user would not understand this message, wouldn't they?

When would base dir even be empty?


return absBaseDir, numberOfRuns


def buildRunDefinitions(benchmark, limits, absBaseDir):
runDefinitions = []
for runSet in activeRunSets(benchmark):
runs = []
for run in runSet.runs:
cmdline = list(map(vcloudutil.force_linux_path, run.cmdline()))
log_file = os.path.relpath(run.log_file, benchmark.log_folder)
run_def = {"logfile": log_file, "command": cmdline}
run_files = []
if os.path.exists(run.identifier):
run_files.extend(run.sourcefiles)
run_files.extend(run.required_files)
if run_files:
run_def["files"] = [os.path.relpath(f, absBaseDir) for f in run_files]
Comment on lines +332 to +334

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this mean that every required file is added to the list for every tasks for which it is required?

IIRC we had problems in the past when the metadata for runs got too big for the BenchCloud's master's memory and we had to change something such that common required files are added only once to the metadata (on the level of the run collection or so), at least if the user was clever enough to specify them on <benchmark> or <rundefinition> tag and not the <tasks> tag in the benchmark definition.

We should make sure not to make this worse now.

runs.append(run_def)
if runs:
runDefinitions.append({"limits": limits, "runs": runs})
return runDefinitions


def activeRunSets(benchmark):
for runSet in benchmark.run_sets:
if STOPPED_BY_INTERRUPT:
break
if runSet.should_be_executed():
yield runSet


def handleCloudResults(benchmark, output_handler, start_time, end_time):
outputDir = benchmark.log_folder
if not os.path.isdir(outputDir) or not os.listdir(outputDir):
Expand Down Expand Up @@ -491,9 +479,3 @@ def read_items():
yield key, value

return vcloudutil.parse_vcloud_run_result(read_items())


def bytes_to_mb(mb):
if mb is None:
return None
return int(mb / 1000 / 1000)