Migrate to YAML input format for sending benchmarking data to BenchCloud#1270
Migrate to YAML input format for sending benchmarking data to BenchCloud#1270AkshayWarrier wants to merge 5 commits into
Conversation
|
Would it be possible to avoid making all paths absolute? I consider this an anti-pattern. Absolute paths may help to avoid path misunderstandings between components, if they are applied everywhere and consistently. But if we have a consistent path handling everywhere, then relative paths with a properly defined base¹ work just as well, so I do not see a real benefit of actual paths. It feels more like "it helped once so now we use absolute paths everywhere". Disadvantages of absolute paths are that eventually they inevitably end up somewhere where they are inconvenient or undesired, for example in logs or result files, and disturb comparisons when debugging, leak private data (user name) on the web, etc. BenchExec for example takes care that it does not introduce absolute paths into output when the user did not use absolute paths as input, and that it never forces the user to use absolute paths as input. I hope BenchCloud does the same. ¹ Standard is that relative paths that are given as command-line arguments or on stdin/stdout are relative to the current working directory, and relative paths given in a file are relative to that file's directory, as long as the involved APIs do not specify something explicitly. |
|
Hi Philipp, so in BenchCloud, all run specific files, and the toolpaths are relativized against the basedir. And I think to relativize them, they both need to be the same kind of path; maybe that's why they were both made absolute. But maybe we can make all these paths absolute on the BenchCloud client side before relativizing them against the base directory? An exception is that the output directory is sent as it is to BenchCloud and is not relativized, so maybe we also just make this absolute on the BenchCloud client side then? |
If you require absolute paths there, why not? An alternative would also be to define for this format that all paths in the file are relative to the given base dir. Though if there is the output directory mixed in, this might not make that much sense. But maybe it is also a good idea to check whether the grouping of inputs is meaningful? E.g., why is the output directory mixed in with the things that define the runs? IIRC there are some things that are passed to the BenchmarkClient as command-line options and some are passed in this file. How is it decided which is where so far? |
I'm not sure how it has been decided so far, but it might make sense to pass the output-dir as a command-line option, separate from the workload. I guess this would also require me to update the legacy format as such.
Do you mean I enforce benchexec to always send all files relative to the base dir (are they already relative?), and on benchcloud assert all paths are relative to base-dir? |
Oh, actually, for backward compatibility, the legacy format would accept the output directory from the input file, but starting with the new format, we would have to expect the output directory to be passed by command-line option. Right? 🤔 |
Would be a possibility. In BenchExec the paths are not relative to the base dir, relative paths are relative to the current working directory.
Yes. At least, it is a possibility, not saying you have to do it. I have no overview of which information is passed to the BenchCloud and how/where it is done, so I also do not know what is the best way to pass the output directory. I am just saying it would be good to have a look at all this information and decide where it fits best when one is anyway completely redesigning the input format. Note that in the past there was no clear design of this, information was just put somewhere where it fit. |
|
Hi Philipp, I've made new changes after our discussion. We decided to pass The The code can be reviewed again :) |
PhilippWendler
left a comment
There was a problem hiding this comment.
Could you please provide a benchmark definition that exercises all relevant features (runs without input files, with more than one input file, required files specified on all levels of the XML, all limits and requirements, result-file patterns, etc.) and both the resulting cloud input in the old and new format? I think this would greatly help to see that there is nothing forgotten in the new format and that everything is consistent.
|
|
||
| absBaseDir = benchexec.util.common_base_dir(absSourceFiles + absToolpaths) | ||
| if absBaseDir == "": | ||
| sys.exit("No common base dir found.") |
There was a problem hiding this comment.
I think a user would not understand this message, wouldn't they?
When would base dir even be empty?
| for run in runSet.runs: | ||
| if os.path.exists(run.identifier): | ||
| absSourceFiles.extend(map(os.path.abspath, run.sourcefiles)) | ||
| numberOfRuns += 1 |
There was a problem hiding this comment.
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 ...).
| break | ||
| # get limits | ||
| rlimits = benchmark.rlimits | ||
| timeLimit = int(rlimits.cputime_hard) |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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 benchmark.result_files_patterns: | ||
| cloud_input["resultFilePatterns"] = list(benchmark.result_files_patterns) | ||
|
|
||
| cloud_input["runsets"] = runDefinitions |
There was a problem hiding this comment.
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 (workingDir, validToolpaths) | ||
|
|
||
|
|
||
| def computeBaseDir(benchmark, absToolpaths): |
There was a problem hiding this comment.
Wouldn't this need to take all files into account that are part of the input, e.g., also the required files?
| if benchmark.result_files_patterns: | ||
| cloud_input["resultFilePatterns"] = list(benchmark.result_files_patterns) |
There was a problem hiding this comment.
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["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] |
There was a problem hiding this comment.
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.
| 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] |
There was a problem hiding this comment.
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.
| return "\t".join(map(str, items)) | ||
|
|
||
|
|
||
| def getCloudInput(benchmark): |
There was a problem hiding this comment.
We need a link here that points to a place where the format for the cloud-input file is described.
The PR migrates the format for sending benchmarking data to Benchcloud from the legacy text form to YAML. It also allows users to pass multiple result file patterns, since the new format now supports it.
Must be merged after https://gitlab.com/sosy-lab/software/benchcloud/-/merge_requests/183 is deployed.