From 333ca7c0e5551eb24a30f96ef969951b12205747 Mon Sep 17 00:00:00 2001 From: Dmitry Petrov Date: Sat, 25 Mar 2017 23:16:49 -0700 Subject: [PATCH] Git wrapper refactoring --- dvc/command/repro.py | 4 +- dvc/command/run.py | 8 +-- dvc/executor.py | 57 +++++++++++++++++++++ dvc/git_wrapper.py | 111 +++++++++++++---------------------------- dvc/state_file.py | 18 +++---- tests/test_executor.py | 51 +++++++++++++++++++ 6 files changed, 157 insertions(+), 92 deletions(-) create mode 100644 dvc/executor.py create mode 100644 tests/test_executor.py diff --git a/dvc/command/repro.py b/dvc/command/repro.py index 9829baf1a..4c4eebd1e 100644 --- a/dvc/command/repro.py +++ b/dvc/command/repro.py @@ -24,7 +24,7 @@ class CmdRepro(CmdRun): pass @property - def code(self): + def code_dependencies(self): return self._code def define_args(self, parser): @@ -157,7 +157,7 @@ class ReproChange(object): were_input_files_changed = True was_source_code_changed = self.git.were_files_changed(self._data_item.data.relative, - self._state.code_sources) + self._state.code_dependencies) if not force and not was_source_code_changed and not were_input_files_changed: Logger.debug('Data file "{}" is up to date'.format( diff --git a/dvc/command/run.py b/dvc/command/run.py index 03693b82c..c4f9fe435 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -34,11 +34,11 @@ class CmdRun(CmdBase): parser.add_argument('--output-file', '-o', action='append', help='Declare output file for reproducible cmd') parser.add_argument('--code', '-c', action='append', - help='Code file or code directory which produces the output') + help='Code dependencies which produce the output') pass @property - def code(self): + def code_dependencies(self): return self.args.code @property @@ -109,13 +109,13 @@ class CmdRun(CmdBase): data_item.create_symlink() - nlx_code_sources = map(lambda x: self.git.abs_paths_to_dvc([x])[0], self.code) + dvc_code_dependencies = map(lambda x: self.git.abs_paths_to_dvc([x])[0], self.code_dependencies) Logger.debug('Create state file "{}"'.format(data_item.state.relative)) state_file = StateFile(data_item.state.relative, self.git, input_files, output_files, - nlx_code_sources, + dvc_code_dependencies, argv=argv) state_file.save() pass diff --git a/dvc/executor.py b/dvc/executor.py new file mode 100644 index 000000000..100a65c1f --- /dev/null +++ b/dvc/executor.py @@ -0,0 +1,57 @@ +import sys +import subprocess + +from dvc.exceptions import DvcException + + +class ExecutorError(DvcException): + def __init__(self, msg): + DvcException.__init__(self, msg) + + +class Executor: + @staticmethod + def exec_cmd(cmd, stdout_file=None, stderr_file=None, cwd=None): + stdout, stdout_fd = Executor.output_file(stdout_file) + stderr, stderr_fd = Executor.output_file(stderr_file) + + try: + p = subprocess.Popen(cmd, + cwd=cwd, + stdout=stdout, + stderr=stderr) + out, err = map(lambda s: s.decode().strip('\n\r') if s else '', p.communicate()) + + return p.returncode, out, err + except Exception as ex: + return 1, None, str(ex) + finally: + if stderr_fd: + stderr_fd.close() + if stdout_fd: + stdout_fd.close() + pass + + @staticmethod + def output_file(output_file, default_output=None): + output_fd = None + if output_file is not None: + if output_file == '-': + output = default_output + else: + output_fd = open(output_file, 'w') + output = output_fd + else: + output = subprocess.PIPE + return output, output_fd + + @staticmethod + def exec_cmd_only_success(cmd, stdout_file=None, stderr_file=None, cwd=None): + code, out, err = Executor.exec_cmd(cmd, stdout_file=stdout_file, stderr_file=stderr_file, cwd=cwd) + if code != 0: + raise Executor._build_executor_error(cmd, err) + return out + + @staticmethod + def _build_executor_error(cmd, out): + return ExecutorError('Git command error ({}):\n{}'.format(' '.join(cmd), out)) diff --git a/dvc/git_wrapper.py b/dvc/git_wrapper.py index 6f55492ee..6a20d95db 100644 --- a/dvc/git_wrapper.py +++ b/dvc/git_wrapper.py @@ -1,15 +1,8 @@ import os -import sys -import subprocess -from dvc.exceptions import DvcException from dvc.logger import Logger from dvc.config import Config - - -class GitCmdError(DvcException): - def __init__(self, msg): - DvcException.__init__(self, msg) +from dvc.executor import Executor, ExecutorError class GitWrapperI(object): @@ -43,50 +36,7 @@ class GitWrapperI(object): class GitWrapper(GitWrapperI): def __init__(self): - GitWrapperI.__init__(self) - - @staticmethod - def exec_cmd(cmd, stdout_file=None, stderr_file=None, cwd=None): - stdout_fd = None - if stdout_file is not None: - if stdout_file == '-': - stdout = sys.stdout - else: - stdout_fd = open(stdout_file, 'w') - stdout = stdout_fd - else: - stdout = subprocess.PIPE - - stderr_fd = None - if stderr_file is not None: - if stderr_file == '-': - stderr = sys.stderr - else: - stderr_fd = open(stderr_file, 'w') - stderr = stderr_fd - else: - stderr = subprocess.PIPE - - p = subprocess.Popen(cmd, cwd=cwd, - stdout=stdout, - stderr=stderr) - out, err = map(lambda s: s.decode().strip('\n\r') if s else '', p.communicate()) - - if stderr_fd: - stderr_fd.close() - if stdout_fd: - stdout_fd.close() - - return p.returncode, out, err - - @staticmethod - def exec_cmd_only_success(cmd, stdout_file=None, stderr_file=None, cwd=None): - code, out, err = GitWrapper.exec_cmd(cmd, stdout_file=stdout_file, stderr_file=stderr_file, cwd=cwd) - if code != 0: - if err: - sys.stderr.write(err + '\n') - raise GitCmdError('Git command error ({}):\n{}'.format(' '.join(cmd), out)) - return out + super(GitWrapper, self).__init__() def is_ready_to_go(self): statuses = self.status_files() @@ -100,7 +50,7 @@ class GitWrapper(GitWrapperI): return True @property - def curr_dir_nlx(self): + def curr_dir_dvc(self): return os.path.relpath(os.path.abspath(os.curdir), self.git_dir_abs) @property @@ -109,24 +59,24 @@ class GitWrapper(GitWrapperI): return self._git_dir try: - code, out, err = GitWrapper.exec_cmd(['git', 'rev-parse', '--show-toplevel']) + code, out, err = Executor.exec_cmd(['git', 'rev-parse', '--show-toplevel']) if code != 0: - raise GitCmdError('Git command error - {}'.format(err)) + raise ExecutorError('Git command error - {}'.format(err)) self._git_dir = out return self._git_dir - except GitCmdError: + except ExecutorError: raise except Exception as e: - raise GitCmdError('Unable to run git command: {}'.format(e)) + raise ExecutorError('Unable to run git command: {}'.format(e)) pass @staticmethod def status_files(): - code, out, err = GitWrapper.exec_cmd(['git', 'status', '--porcelain']) + code, out, err = Executor.exec_cmd(['git', 'status', '--porcelain']) if code != 0: - raise GitCmdError('Git command error - {}'.format(err)) + raise ExecutorError('Git command error - {}'.format(err)) return GitWrapper.parse_porcelain_files(out) @@ -144,16 +94,17 @@ class GitWrapper(GitWrapperI): @property def curr_commit(self): if self._commit is None: - code, out, err = GitWrapper.exec_cmd(['git', 'rev-parse', '--short', 'HEAD']) + code, out, err = Executor.exec_cmd(['git', 'rev-parse', '--short', 'HEAD']) if code != 0: - raise GitCmdError('Git command error - {}'.format(err)) + raise ExecutorError('Git command error - {}'.format(err)) self._commit = out return self._commit - def commit_all_changes(self, message): - GitWrapper.exec_cmd_only_success(['git', 'add', '--all']) - out_status = GitWrapper.exec_cmd_only_success(['git', 'status', '--porcelain']) - GitWrapper.exec_cmd_only_success(['git', 'commit', '-m', message]) + @staticmethod + def commit_all_changes(message): + Executor.exec_cmd_only_success(['git', 'add', '--all']) + out_status = Executor.exec_cmd_only_success(['git', 'status', '--porcelain']) + Executor.exec_cmd_only_success(['git', 'commit', '-m', message]) return GitWrapper.parse_porcelain_files(out_status) def commit_all_changes_and_log_status(self, message): @@ -180,7 +131,7 @@ class GitWrapper(GitWrapperI): return result - def nlx_paths_to_abs(self, files): + def dvc_paths_to_abs(self, files): results = [] for file in files: @@ -188,21 +139,14 @@ class GitWrapper(GitWrapperI): return results - def were_files_changed(self, file, code_sources): - commit = self.exec_cmd_only_success(['git', 'log', '-1', '--pretty=format:"%h"', file]) + def were_files_changed(self, file, code_dependencies): + commit = Executor.exec_cmd_only_success(['git', 'log', '-1', '--pretty=format:"%h"', file]) commit = commit.strip('"') - changed_files = self.exec_cmd_only_success(['git', 'diff', '--name-only', 'HEAD', commit]) + changed_files = Executor.exec_cmd_only_success(['git', 'diff', '--name-only', 'HEAD', commit]) changed_files = changed_files.strip('"') - code_sources_abs = self.nlx_paths_to_abs(code_sources) - code_files = [] - code_dirs = [] - for code in code_sources_abs: - if os.path.isdir(code): - code_dirs.append(code) - else: - code_files.append(code) + code_files, code_dirs = self.separate_dependency_files_and_dirs(code_dependencies) code_files_set = set(code_files) for changed_file in changed_files.split('\n'): @@ -215,3 +159,16 @@ class GitWrapper(GitWrapperI): return True return False + + def separate_dependency_files_and_dirs(self, code_dependencies): + code_files = [] + code_dirs = [] + + code_dependencies_abs = self.dvc_paths_to_abs(code_dependencies) + for code in code_dependencies_abs: + if os.path.isdir(code): + code_dirs.append(code) + else: + code_files.append(code) + + return code_files, code_dirs diff --git a/dvc/state_file.py b/dvc/state_file.py index f002ce5f6..7e2d6cad2 100644 --- a/dvc/state_file.py +++ b/dvc/state_file.py @@ -23,11 +23,11 @@ class StateFile(object): PARAM_CREATED_AT = 'CreatedAt' PARAM_INPUT_FILES = 'InputFiles' PARAM_OUTPUT_FILES = 'OutputFiles' - PARAM_CODE_SOURCES = 'CodeSources' + PARAM_CODE_DEPENDENCIES = 'CodeDependencies' PARAM_NOT_REPRODUCIBLE = 'NotReproducible' def __init__(self, file, git, input_files, output_files, - code_sources=[], + code_dependencies=[], is_reproducible=True, argv=sys.argv, norm_argv=None, @@ -38,7 +38,7 @@ class StateFile(object): self.input_files = input_files self.output_files = output_files self.is_reproducible = is_reproducible - self.code_sources = code_sources + self.code_dependencies = code_dependencies self.argv = argv if norm_argv: @@ -51,7 +51,7 @@ class StateFile(object): if cwd: self.cwd = cwd else: - self.cwd = self.get_nlx_path() + self.cwd = self.get_dvc_path() pass @staticmethod @@ -63,7 +63,7 @@ class StateFile(object): git, data.get(StateFile.PARAM_INPUT_FILES, []), data.get(StateFile.PARAM_OUTPUT_FILES, []), - data.get(StateFile.PARAM_CODE_SOURCES, []), + data.get(StateFile.PARAM_CODE_DEPENDENCIES, []), not data.get(StateFile.PARAM_NOT_REPRODUCIBLE, False), data.get(StateFile.PARAM_ARGV), data.get(StateFile.PARAM_NORM_ARGV), @@ -80,7 +80,7 @@ class StateFile(object): self.PARAM_CREATED_AT: self.created_at, self.PARAM_INPUT_FILES: self.input_files, self.PARAM_OUTPUT_FILES: self.output_files, - self.PARAM_CODE_SOURCES: self.code_sources + self.PARAM_CODE_DEPENDENCIES: self.code_dependencies } if not self.is_reproducible: @@ -107,14 +107,14 @@ class StateFile(object): for arg in self.argv [1:]: if os.path.isfile(arg): path = os.path.abspath(arg) - nlx_path = os.path.relpath(path, self.git.git_dir_abs) - result.append(nlx_path) + dvc_path = os.path.relpath(path, self.git.git_dir_abs) + result.append(dvc_path) else: result.append(arg) return result - def get_nlx_path(self): + def get_dvc_path(self): pwd = os.path.realpath(os.curdir) if not pwd.startswith(self.git.git_dir_abs): raise StateFileError('the file cannot be created outside of a git repository') diff --git a/tests/test_executor.py b/tests/test_executor.py new file mode 100644 index 000000000..5248a6183 --- /dev/null +++ b/tests/test_executor.py @@ -0,0 +1,51 @@ +import os +import shutil +import tempfile +from unittest import TestCase + +from dvc.executor import Executor, ExecutorError + + +class TestExecutor(TestCase): + def setUp(self): + self.test_dir = tempfile.mkdtemp() + + self.file1 = 'file1.txt' + fd1 = open(os.path.join(self.test_dir, self.file1), 'w+') + fd1.write('this is file 1') + fd1.close() + + self.file2 = 'f.csv' + fd2 = open(os.path.join(self.test_dir, self.file2), 'w+') + fd2.write('this is file 222') + fd2.close() + + def tearDown(self): + shutil.rmtree(self.test_dir) + + def test_suppress_output(self): + output = Executor.exec_cmd_only_success(['ls', self.test_dir]) + output_set = set(output.split()) + self.assertEqual(output_set, {self.file1, self.file2}) + + def test_output_to_std_console(self): + output = Executor.exec_cmd_only_success(['ls', self.test_dir], '-', '-') + self.assertEqual(output, '') + + def test_custom_files_output(self): + stdout_file = os.path.join(self.test_dir, 'stdout.txt') + stderr_file = os.path.join(self.test_dir, 'stderr.txt') + output = Executor.exec_cmd_only_success(['ls', self.test_dir], stdout_file, stderr_file) + + self.assertEqual(output, '') + + output_set = set(open(stdout_file).read().split()) + expected = {self.file1, self.file2, os.path.basename(stdout_file), os.path.basename(stderr_file)} + self.assertEqual(output_set, expected) + + os.remove(stdout_file) + os.remove(stderr_file) + + def test_wrong_command(self): + with self.assertRaises(ExecutorError): + Executor.exec_cmd_only_success(['--wrong-command--', self.test_dir])