Conversation
smarr
left a comment
smarr
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Some initial thoughts as individual comments.
rebench/configurator.py
Outdated
| def _parse_executor(self, executor_name, executor_config): | ||
| path = executor_config.get('path') | ||
| executable = executor_config.get('executable') | ||
| cores = executor_config.get('cores') | ||
| version_command = executor_config.get('version_command') | ||
|
|
||
| executor = Executor([], False, self.ui, config_dir=self.config_dir) | ||
| if version_command: | ||
| executor.set_version_command(version_command) | ||
| return executor | ||
|
|
||
| def _compile_experiment(self, exp_name, experiment): | ||
| experiment['executors'] = {name: self._parse_executor(name, config) for name, config in | ||
| experiment.get('executors', {}).items()} | ||
| return Experiment.compile(exp_name, experiment, self) | ||
|
|
There was a problem hiding this comment.
Could you elaborate a bit what this code is doing?
Isn't this what model.executor.Executor.compile() is doing?
rebench/configurator.py
Outdated
| executor_config = self._executors[executor_name] | ||
| executor = Executor.compile( | ||
| executor_name, self._executors[executor_name], | ||
| executor_name, executor_config, | ||
| run_details, variables, self.build_commands, action) | ||
|
|
||
| version_command = executor_config.get('version_command') | ||
| if version_command: | ||
| executor.version_command = version_command | ||
|
|
There was a problem hiding this comment.
This also looks like it should be handled in model.executor.Executor.compile()
| """Specializing the executor details in the run definitions with the settings from | ||
| the executor definitions | ||
| """ |
There was a problem hiding this comment.
Is there a reason for removing this docstring?
rebench/model/executor.py
Outdated
| except subprocess.CalledProcessError as e: | ||
| return e.stderr.strip() | ||
| else: | ||
| return "Unknown version" |
There was a problem hiding this comment.
Could we use None instead of a string here? Otherwise, we won't be able to programmatically tell apart the cases of not having a version and having a string with arbitrary text.
| executors: | ||
| TestRunner1: | ||
| path: ~/PycharmProjects/ReBench/rebench/tests | ||
| executable: test-vm1.py %(cores)s | ||
| cores: [1] | ||
| version_command: "python --version" | ||
|
|
There was a problem hiding this comment.
Do we really need three new config files for this? Could this perhaps be solved with having 3 different executors in the same config file and testing directly for those?
Handle version: _command, _string, _git. Removed code from configurator.py, which duplicates executor.py. Joined all separate “version” configuration files into a single one. Also, in the executor.py get_version function returns None instead of string, if version is not found.
Handle version: _command, _string, _git. Removed code from configurator.py, which duplicates executor.py. Joined all separate “version” configuration files into a single one. Also, in the executor.py get_version function returns None instead of string, if version is not found. Also fixed the code in order to make it pass the pylit tests (line too long, etc.)
2 participants
Handle version_string, version_git.