From a813ebaeb771104efb59ce6df18fdb7f6241bc4f Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Thu, 26 Oct 2017 15:28:46 +0300 Subject: [PATCH 01/15] dvc: use hardlinks instead of symlinks Fixes #225 Fixes #273 Signed-off-by: Ruslan Kuprieiev --- dvc/command/import_file.py | 3 +-- dvc/command/init.py | 24 ++++----------------- dvc/command/repro.py | 7 ++----- dvc/command/run.py | 17 +++++++-------- dvc/command/traverse.py | 2 +- dvc/data_cloud.py | 16 ++++++++++---- dvc/git_wrapper.py | 11 ++++++++++ dvc/path/data_item.py | 17 ++++++--------- dvc/path/factory.py | 15 ++++++++----- dvc/system.py | 43 +++----------------------------------- 10 files changed, 57 insertions(+), 98 deletions(-) diff --git a/dvc/command/import_file.py b/dvc/command/import_file.py index 2ff486398..9bcb3d304 100644 --- a/dvc/command/import_file.py +++ b/dvc/command/import_file.py @@ -132,8 +132,7 @@ class CmdImportFile(CmdBase): Logger.debug('Skipping creating state file for failed import {}'.format(data_item.state.relative)) continue - Logger.debug('Creating symlink {} --> {}'.format(data_item.symlink_file, data_item.data.relative)) - System.symlink(data_item.symlink_file, data_item.data.relative) + Logger.debug('Creating state file for {}'.format(data_item.data.relative)) state_file = StateFile(StateFile.COMMAND_IMPORT_FILE, data_item, diff --git a/dvc/command/init.py b/dvc/command/init.py index 2e3568cdc..d0ddeb2cf 100644 --- a/dvc/command/init.py +++ b/dvc/command/init.py @@ -116,16 +116,11 @@ ProjectName = conf_file.write(self.CONFIG_TEMPLATE.format(data_dir_path.name)) conf_file.close() - message = 'DVC init. data dir {}, cache dir {}, state dir {}, '.format( - data_dir_path.name, - cache_dir_path.name, - state_dir_path.name - ) - if self.commit_if_needed(message) == 1: - return 1 + self.git.modify_gitignore([os.path.join(config_dir_path.name, cache_dir_path.name), + os.path.join(config_dir_path.name, os.path.basename(self.git.lock_file))]) - self.modify_gitignore(config_dir_path.name, cache_dir_path.name) - return self.commit_if_needed('DVC init. Commit .gitignore file') + message = 'DVC init. cache dir {}, state dir {}, '.format(cache_dir_path.name, state_dir_path.name) + return self.commit_if_needed(message) def create_empty_file(self): empty_data_path = os.path.join(self.parsed_args.data_dir, self.EMPTY_FILE_NAME) @@ -141,14 +136,3 @@ ProjectName = output_files=[], lock=False).save(is_update_target_metrics=False) pass - - def modify_gitignore(self, config_dir_name, cache_dir_name): - gitignore_file = os.path.join(self.git.git_dir, '.gitignore') - if not os.path.exists(gitignore_file): - open(gitignore_file, 'a').close() - Logger.info('File .gitignore was created') - with open(gitignore_file, 'a') as fd: - fd.write('\n{}'.format(os.path.join(config_dir_name, cache_dir_name))) - fd.write('\n{}'.format(os.path.join(config_dir_name, os.path.basename(self.git.lock_file)))) - - Logger.info('Directory {} was added to .gitignore file'.format(cache_dir_name)) diff --git a/dvc/command/repro.py b/dvc/command/repro.py index f618cd341..ba1cbdb1d 100644 --- a/dvc/command/repro.py +++ b/dvc/command/repro.py @@ -119,9 +119,6 @@ class ReproChange(object): self._recursive = recursive self._force = force - if not System.islink(data_item.data.relative): - raise ReproError('data item {} is not symlink'.format(data_item.data.relative)) - try: self._state = StateFile.load(data_item, self.git) except Exception as ex: @@ -246,8 +243,8 @@ class ReproChange(object): result = True Logger.debug(u'Repro data item {}: dependency {} was changed'.format( data_item_dvc, data_item.data.dvc)) - elif data_item.data.dvc in self._globally_changed_files: - msg = u'Repro data item {}: dependency {} was not changed but the data item global checksum was changed' + elif data_item.state.dvc in self._globally_changed_files: + msg = u'Repro data item {}: dependency {} was not changed but the state file was changed' Logger.debug(msg.format(data_item_dvc, data_item.data.dvc)) result = True else: diff --git a/dvc/command/run.py b/dvc/command/run.py index 9c8a94de7..51759977b 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -116,7 +116,7 @@ class CmdRun(CmdBase): result = [] for data_item in repo_change.changed_data_items: - Logger.debug('Move output file "{}" to cache dir "{}" and create a symlink'.format( + Logger.debug('Move output file "{}" to cache dir "{}" and create a hardlink'.format( data_item.data.relative, data_item.cache_dir)) data_item.move_data_to_cache() @@ -168,16 +168,13 @@ class CmdRun(CmdBase): not_data_items = [] for arg in argv: - try: - if os.path.isfile(arg): - data_item = self.settings.path_factory.data_item(arg) - data_items.append(data_item) - except NotInGitDirError as ex: - msg = 'File {} from argv is outside of git directory and cannot be traced: {}' - Logger.warn(msg.format(arg, ex)) - except NotInDataDirError: + if self.settings.path_factory.is_data_item(arg): + data_items.append(self.settings.path_factory.data_item(arg)) + elif os.path.isfile(arg): not_data_items.append(arg) - pass + else: + msg = 'File {} from argv is outside of git directory and cannot be traced' + Logger.warn(msg.format(arg)) return data_items, not_data_items diff --git a/dvc/command/traverse.py b/dvc/command/traverse.py index 63e030521..6958d8241 100644 --- a/dvc/command/traverse.py +++ b/dvc/command/traverse.py @@ -72,7 +72,7 @@ class Traverse(CmdBase): try: data_item = self.settings.path_factory.existing_data_item(target) except DataItemError: - Logger.warn(u'[TraverseFileTree] Data file {} is not valid symbolic link'.format(target)) + Logger.warn(u'[TraverseFileTree] Data file {} does not have existing state file'.format(target)) data_item = self.settings.path_factory.data_item(target) return data_item diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index 5a84692d0..25ae03a26 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -219,11 +219,13 @@ class DataCloudLOCAL(DataCloudBase): tmp_file = self.tmp_file(item.data.dvc) try: copyfile(i, tmp_file) - item.import_cache(tmp_file) except Exception as exc: Logger.error('Failed to copy "{}": {}'.format(i, exc)) return None + os.rename(tmp_file, item.data.dvc) + item.move_data_to_cache() + return item def pull(self, item): @@ -363,7 +365,8 @@ class DataCloudHTTP(DataCloudBase): if not self._verify_md5(req, tmp_file): return None - item.import_cache(tmp_file) + os.rename(tmp_file, item.data.dvc) + item.move_data_to_cache() return item @@ -476,11 +479,13 @@ class DataCloudAWS(DataCloudBase): num_retries=10) try: key.get_contents_to_filename(tmp_file, cb=create_cb(name), res_download_handler=res_h) - data_item.import_cache(tmp_file) except Exception as exc: Logger.error('Failed to download "{}": {}'.format(key_name, exc)) return None + os.rename(tmp_file, fname) + data_item.move_data_to_cache() + progress.finish_target(name) Logger.debug('Downloading completed') @@ -726,6 +731,9 @@ class DataCloudGCP(DataCloudBase): Logger.error('Failed to download "{}": {}'.format(key, exc)) return None + os.rename(tmp_file, fname) + data_item.move_data_to_cache() + progress.finish_target(name) Logger.debug('Downloading completed') @@ -877,7 +885,7 @@ class DataCloud(object): """ Collect target as a file or directory. """ - if System.islink(target): + if self._settings.path_factory.is_data_item(target): item = self._settings.path_factory.data_item(target) return [item] elif os.path.isdir(target): diff --git a/dvc/git_wrapper.py b/dvc/git_wrapper.py index 1b309ac8b..ff3a41341 100644 --- a/dvc/git_wrapper.py +++ b/dvc/git_wrapper.py @@ -96,6 +96,17 @@ class GitWrapperI(object): def commit_all_changes_and_log_status(self, message): pass + @staticmethod + def modify_gitignore(flist): + for fname in flist: + # We're modifying .gitignore in the same dir as the file itself, so + # that if dir consists only of data files it will be still tracked by + # git and we will not have any problems with --porcelain output for + # specific changed files. + gitignore = os.path.join(os.path.dirname(fname), '.gitignore') + with open(gitignore, 'a') as fd: + fd.write('\n' + os.path.basename(fname)) + class GitWrapper(GitWrapperI): FLOATS_FROM_STRING = re.compile(r'[-+]?(?:(?:\d*\.\d+)|(?:\d+\.?))(?:[Ee][+-]?\d+)?') diff --git a/dvc/path/data_item.py b/dvc/path/data_item.py index 6719a75a5..e5478f7a5 100644 --- a/dvc/path/data_item.py +++ b/dvc/path/data_item.py @@ -125,15 +125,10 @@ class DataItem(object): def state_dir_abs(self): return os.path.join(self._git.git_dir_abs, self._config.state_dir) - @property - def symlink_file(self): - data_file_dir = os.path.dirname(self.data.relative) - return os.path.relpath(self.cache.relative, data_file_dir) - def move_data_to_cache(self): - self.import_cache(self.data.relative) - System.symlink(self.symlink_file, self.data.relative) - - def import_cache(self, fname): - self._cache_file = os.path.join(self.cache_dir, file_md5(fname)[0]) - os.rename(fname, self._cache_file) + md5 = file_md5(self.data.relative)[0] + self._cache_file = os.path.join(self.cache_dir, md5) + self._git.modify_gitignore([self.data.relative]) + if os.path.isfile(self.cache.relative): + return + System.hardlink(self.data.relative, self.cache.relative) diff --git a/dvc/path/factory.py b/dvc/path/factory.py index 23f20e0d2..68078f74a 100644 --- a/dvc/path/factory.py +++ b/dvc/path/factory.py @@ -25,11 +25,16 @@ class PathFactory(object): path = Path.from_dvc_path(dvc_path, self._git) return self.existing_data_item(path.relative) - def existing_data_item(self, file): - if not System.islink(file): - raise DataItemError(u'Data file "%s" must be a symbolic link' % file) - resolved_symlink = System.realpath(file) - return DataItem(file, self._git, self._config, resolved_symlink) + def is_data_item(self, fname): + data = os.path.relpath(os.path.realpath(fname), self._config.data_dir) + state = os.path.join(self._config.state_dir, data + DataItem.STATE_FILE_SUFFIX) + return os.path.isfile(state) + + def existing_data_item(self, fname): + if not self.is_data_item(fname): + raise DataItemError(u'No state file found for data file {}'.format(fname)) + + return DataItem(fname, self._git, self._config) def to_data_items(self, files): result = [] diff --git a/dvc/system.py b/dvc/system.py index 43c52053a..bb4d41acc 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -10,7 +10,6 @@ from dvc.executor import Executor class System(object): - SYMLINK_OUTPUT = '' LONG_PATH_BUFFER_SIZE = 1024 @staticmethod @@ -18,47 +17,11 @@ class System(object): return os.name != 'nt' @staticmethod - def symlink(source, link_name): + def hardlink(source, link_name): if System.is_unix(): - return os.symlink(source, link_name) + return os.link(source, link_name) - flags = 0 - if source is not None and os.path.isdir(source): - flags = 1 - - func = ctypes.windll.kernel32.CreateSymbolicLinkW - func.argtypes = (ctypes.c_wchar_p, ctypes.c_wchar_p, ctypes.c_uint32) - func.restype = ctypes.c_ubyte - - if func(link_name, source, flags) == 0: - raise ctypes.WinError() - - @staticmethod - def islink(path): - # It is definitely not the best way to check a symlink. - - if System.is_unix(): - return os.path.islink(path) - - if not os.path.exists(path): - return False - - if not os.path.isfile(path): - return False - - return System._get_symlink_string(path) is not None - - @staticmethod - def _get_symlink_string(path): - code, output, _ = Executor.exec_cmd(["dir", path], shell=True) - if code != 0: - return None - - lines = output.split('\n') - for line in lines: - if System.SYMLINK_OUTPUT in line: - return line - return None + raise Exception('Not Implemented Yet') @staticmethod def realpath(path): From 4ffac507df3366a1e8d8f5cf5b9b758fa33d97cb Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Wed, 25 Oct 2017 14:27:56 +0300 Subject: [PATCH 02/15] dvc: remove data dir From now on, we're no longer tied to fixed data dir, so we can track individual data items without forcing user to use separate dir for them. This way we add much more flexibility for dvc, so users can use their own project structure. Fixes #68 Signed-off-by: Ruslan Kuprieiev --- dvc/command/init.py | 13 +++---------- dvc/command/merge.py | 1 - dvc/command/repro.py | 8 ++++---- dvc/command/run.py | 2 +- dvc/command/show_pipeline.py | 6 +++--- dvc/command/traverse.py | 4 ---- dvc/config.py | 16 ++++------------ dvc/path/data_item.py | 36 ++++++++++++------------------------ dvc/path/factory.py | 6 +++--- dvc/path/stated_data_item.py | 2 +- dvc/repository_change.py | 6 +++--- dvc/state_file.py | 4 ++-- 12 files changed, 36 insertions(+), 68 deletions(-) diff --git a/dvc/command/init.py b/dvc/command/init.py index d0ddeb2cf..73aee7603 100644 --- a/dvc/command/init.py +++ b/dvc/command/init.py @@ -16,8 +16,6 @@ class InitError(DvcException): class CmdInit(CmdBase): CONFIG_TEMPLATE = '''[Global] -DataDir = {} - # Default target Target = @@ -92,28 +90,23 @@ ProjectName = return 1 config_dir_path = self.get_not_existing_path(Config.CONFIG_DIR) - data_dir_path = self.get_not_existing_path(self.parsed_args.data_dir) cache_dir_path = self.get_not_existing_path(Config.CONFIG_DIR, Config.CACHE_DIR) state_dir_path = self.get_not_existing_path(Config.CONFIG_DIR, Config.STATE_DIR) - self.settings.config.set(self.parsed_args.data_dir) - conf_file_name = self.get_not_existing_conf_file_name() config_dir_path.mkdir() - data_dir_path.mkdir() cache_dir_path.mkdir() state_dir_path.mkdir() - Logger.info('Directories {}/, {}/, {}/, {}/ were created'.format( + Logger.info('Directories {}/, {}/, {}/ were created'.format( config_dir_path.name, - data_dir_path.name, os.path.join(config_dir_path.name, cache_dir_path.name), os.path.join(config_dir_path.name, state_dir_path.name))) self.create_empty_file() conf_file = open(conf_file_name, 'wt') - conf_file.write(self.CONFIG_TEMPLATE.format(data_dir_path.name)) + conf_file.write(self.CONFIG_TEMPLATE) conf_file.close() self.git.modify_gitignore([os.path.join(config_dir_path.name, cache_dir_path.name), @@ -123,7 +116,7 @@ ProjectName = return self.commit_if_needed(message) def create_empty_file(self): - empty_data_path = os.path.join(self.parsed_args.data_dir, self.EMPTY_FILE_NAME) + empty_data_path = os.path.join(self.settings.git.git_dir_abs, self.EMPTY_FILE_NAME) data_item = self.settings.path_factory.data_item(empty_data_path) open(empty_data_path, 'w').close() diff --git a/dvc/command/merge.py b/dvc/command/merge.py index 13afab86b..f730aa784 100644 --- a/dvc/command/merge.py +++ b/dvc/command/merge.py @@ -3,7 +3,6 @@ import json from dvc.command.base import CmdBase, DvcLock from dvc.logger import Logger from dvc.state_file import StateFile -from dvc.path.data_item import NotInDataDirError class CmdMerge(CmdBase): def __init__(self, settings): diff --git a/dvc/command/repro.py b/dvc/command/repro.py index ba1cbdb1d..16b7f1763 100644 --- a/dvc/command/repro.py +++ b/dvc/command/repro.py @@ -6,7 +6,7 @@ from dvc.command.import_file import CmdImportFile from dvc.command.run import CmdRun from dvc.logger import Logger from dvc.exceptions import DvcException -from dvc.path.data_item import NotInDataDirError +from dvc.path.data_item import DataDirError from dvc.state_file import StateFile from dvc.system import System @@ -64,8 +64,8 @@ class CmdRepro(CmdRun): data_item_list, external_files_names = self.settings.path_factory.to_data_items(target) if external_files_names: - Logger.error('Files from outside of the data directory "{}" could not be reproduced: {}'. - format(self.config.data_dir, ' '.join(external_files_names))) + Logger.error('Files from outside of the repo could not be reproduced: {}'. + format(' '.join(external_files_names))) return 1 if self.repro_data_items(data_item_list, recursive, force): @@ -303,7 +303,7 @@ class ReproChange(object): for input_file in self.state.input_files: try: data_item = self._settings.path_factory.data_item(input_file) - except NotInDataDirError: + except DataDirError: raise ReproError(u'The dependency file "{}" is not a data item'.format(input_file)) except Exception as ex: raise ReproError(u'Unable to reproduced the dependency file "{}": {}'.format( diff --git a/dvc/command/run.py b/dvc/command/run.py index 51759977b..498d1b2a5 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -5,7 +5,7 @@ from dvc.command.base import CmdBase, DvcLock from dvc.exceptions import DvcException from dvc.git_wrapper import GitWrapper from dvc.logger import Logger -from dvc.path.data_item import NotInDataDirError, NotInGitDirError +from dvc.path.data_item import DataDirError, NotInGitDirError from dvc.repository_change import RepositoryChange from dvc.state_file import StateFile from dvc.utils import cached_property diff --git a/dvc/command/show_pipeline.py b/dvc/command/show_pipeline.py index 55f0c6e03..6f7d54b48 100644 --- a/dvc/command/show_pipeline.py +++ b/dvc/command/show_pipeline.py @@ -34,7 +34,7 @@ class CmdShowPipeline(Traverse): def draw_targets(self, target): if not target: - target = self.settings.config.data_dir + target = '.' return self.draw(self.g, target, target) for t in self.parsed_args.target: @@ -49,7 +49,7 @@ class CmdShowPipeline(Traverse): def run(self): saved_targets = self.settings.parsed_args.target - self.settings.parsed_args.target = [self.settings.config.data_dir] + self.settings.parsed_args.target = ['.'] ret = super(CmdShowPipeline, self).run() @@ -70,7 +70,7 @@ class CmdShowPipeline(Traverse): name = data_item.data.relative state = StateFile.load(data_item, self.git) - if name == os.path.join(self.settings.config.data_dir, CmdInit.EMPTY_FILE_NAME): + if name == os.path.join('.', CmdInit.EMPTY_FILE_NAME): return self.g.add_node(name) diff --git a/dvc/command/traverse.py b/dvc/command/traverse.py index 6958d8241..2a29ab796 100644 --- a/dvc/command/traverse.py +++ b/dvc/command/traverse.py @@ -44,10 +44,6 @@ class Traverse(CmdBase): msg = '[TraverseFileTree] Directory "%s" cannot be traversed. Use --recurcive flag.' raise TraverseError(msg % target) - if self._do_not_start_from_root and self.settings.path_factory.data_item(target).data_dvc_short == '': - # The entire data directory - raise TraverseError('[Traverse] Root data directory "%s" cannot be traversed' % target) - self._traverse_dir(target) else: self.process_file(target) diff --git a/dvc/config.py b/dvc/config.py index d2b890d93..cff3b41e0 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -22,23 +22,16 @@ class ConfigI(object): STATE_DIR = 'state' CACHE_DIR = 'cache' - def __init__(self, data_dir=None, cloud=None, conf_parser=None): - self._data_dir = None + def __init__(self, cloud=None, conf_parser=None): self._conf_parser = None self._cloud = None - self.set(data_dir, cloud, conf_parser) + self.set(cloud, conf_parser) - def set(self, data_dir, cloud=None, conf_parser=None): + def set(self, cloud=None, conf_parser=None): """ Set config params """ - self._data_dir = data_dir self._cloud = cloud self._conf_parser = conf_parser - @property - def data_dir(self): - """ Directory with data files """ - return self._data_dir - @property def cache_dir(self): """ Directory with cached data files """ @@ -82,8 +75,7 @@ class Config(ConfigI): level = self._config['Global']['LogLevel'] Logger.set_level(level) - super(Config, self).__init__(self._config['Global']['DataDir'], - self._config['Global']['Cloud'], + super(Config, self).__init__(self._config['Global']['Cloud'], self._config) @property diff --git a/dvc/path/data_item.py b/dvc/path/data_item.py index e5478f7a5..1620868ed 100644 --- a/dvc/path/data_item.py +++ b/dvc/path/data_item.py @@ -13,22 +13,21 @@ class DataItemError(DvcException): super(DataItemError, self).__init__('Data item error: {}'.format(msg)) -class NotInDataDirError(DvcException): - def __init__(self, file, data_dir, pattern='the file "{}" is not in the data directory "{}"'): - msg = pattern.format(file, data_dir) - super(NotInDataDirError, self).__init__(msg) +class DataDirError(DvcException): + def __init__(self, msg): + super(DataDirError, self).__init__(msg) -class DataItemInStatusDirError(NotInDataDirError): - def __init__(self, file, data_dir): - pattern = 'the file "{}" is in state directory, not in the data directory "{}"' - super(DataItemInStatusDirError, self).__init__(file, data_dir, pattern) +class DataItemInStatusDirError(DataDirError): + def __init__(self, file): + msg = 'File "{}" is in state directory'.format(file) + super(DataItemInStatusDirError, self).__init__(msg) -class NotInGitDirError(NotInDataDirError): +class NotInGitDirError(DataDirError): def __init__(self, file, git_dir): - pattern = 'the file "{}" is not in git directory "{}"' - super(NotInGitDirError, self).__init__(file, git_dir, pattern) + msg = 'File "{}" is not in git directory "{}"'.format(file, git_dir) + super(NotInGitDirError, self).__init__(msg) class DataItem(object): @@ -46,11 +45,8 @@ class DataItem(object): raise NotInGitDirError(data_file, self._git.git_dir_abs) if self._data.abs.startswith(self.state_dir_abs): - raise DataItemInStatusDirError(data_file, self._config.data_dir) + raise DataItemInStatusDirError(data_file) - if not self._data.abs.startswith(self.data_dir_abs) or \ - not self._data.dvc.startswith(self._config.data_dir): - raise NotInDataDirError(data_file, self._config.data_dir) pass def copy(self, cache_file=None): @@ -72,14 +68,10 @@ class DataItem(object): def data(self): return self._data - @property - def data_dvc_short(self): - return self._data.dvc[len(self._config.data_dir)+1:] - @cached_property def state(self): state_dir = os.path.join(self._git.git_dir_abs, self._config.state_dir) - state_file = os.path.join(state_dir, self.data_dvc_short + self.STATE_FILE_SUFFIX) + state_file = os.path.join(state_dir, self.data.dvc + self.STATE_FILE_SUFFIX) return Path(state_file, self._git) @cached_property @@ -117,10 +109,6 @@ class DataItem(object): resolved_cache = os.path.realpath(self._data.relative) return Path(resolved_cache, self._git) - @cached_property - def data_dir_abs(self): - return os.path.join(self._git.git_dir_abs, self._config.data_dir) - @cached_property def state_dir_abs(self): return os.path.join(self._git.git_dir_abs, self._config.state_dir) diff --git a/dvc/path/factory.py b/dvc/path/factory.py index 68078f74a..1ca938cc5 100644 --- a/dvc/path/factory.py +++ b/dvc/path/factory.py @@ -1,6 +1,6 @@ import os -from dvc.path.data_item import DataItem, DataItemError, NotInDataDirError +from dvc.path.data_item import DataItem, DataItemError, DataDirError from dvc.path.path import Path from dvc.path.stated_data_item import StatedDataItem from dvc.system import System @@ -26,7 +26,7 @@ class PathFactory(object): return self.existing_data_item(path.relative) def is_data_item(self, fname): - data = os.path.relpath(os.path.realpath(fname), self._config.data_dir) + data = os.path.relpath(os.path.realpath(fname), self._git.git_dir_abs) state = os.path.join(self._config.state_dir, data + DataItem.STATE_FILE_SUFFIX) return os.path.isfile(state) @@ -44,7 +44,7 @@ class PathFactory(object): try: data_item = DataItem(file, self._git, self._config) result.append(data_item) - except NotInDataDirError: + except DataDirError: externally_created_files.append(file) return result, externally_created_files diff --git a/dvc/path/stated_data_item.py b/dvc/path/stated_data_item.py index 01fef1532..15feb3ba6 100644 --- a/dvc/path/stated_data_item.py +++ b/dvc/path/stated_data_item.py @@ -36,4 +36,4 @@ class StatedDataItem(DataItem): return self.is_new or self.is_modified or self.is_removed def __repr__(self): - return u'({}, {})'.format(self.state, self.data_dvc_short) + return u'({}, {})'.format(self.state, self.data.dvc) diff --git a/dvc/repository_change.py b/dvc/repository_change.py index 10aad3d25..c34916a92 100644 --- a/dvc/repository_change.py +++ b/dvc/repository_change.py @@ -4,7 +4,7 @@ from dvc.exceptions import DvcException from dvc.git_wrapper import GitWrapper from dvc.executor import Executor from dvc.logger import Logger -from dvc.path.data_item import NotInDataDirError, DataItemInStatusDirError +from dvc.path.data_item import DataDirError, DataItemInStatusDirError from dvc.path.stated_data_item import StatedDataItem from dvc.utils import cached_property @@ -76,7 +76,7 @@ class RepositoryChange(object): item.data.dvc)) except DataItemInStatusDirError: self._created_status_files.append(file) - except NotInDataDirError: + except DataDirError: self._externally_created_files.append(file) pass @@ -116,7 +116,7 @@ class RepositoryChange(object): def data_file_timesteps(self): res = set() - for root, dirs, files in os.walk(self._settings.config.data_dir): + for root, dirs, files in os.walk('.'): for file in files: filename = os.path.join(root, file) if os.path.exists(filename): diff --git a/dvc/state_file.py b/dvc/state_file.py index 22004fb52..2e1bed6b5 100644 --- a/dvc/state_file.py +++ b/dvc/state_file.py @@ -5,7 +5,7 @@ import json import re from dvc.exceptions import DvcException -from dvc.path.data_item import NotInDataDirError +from dvc.path.data_item import DataDirError from dvc.system import System @@ -248,7 +248,7 @@ class StateFile(object): try: data_item = self.settings.path_factory.data_item(arg) result.append(data_item.data.dvc) - except NotInDataDirError: + except DataDirError: result.append(arg) return result From 1d109b41a171b137d68cf2dff0778f14f26827dc Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 29 Oct 2017 20:14:47 +0200 Subject: [PATCH 03/15] dvc: fix gc Fixes #271 Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 3 --- dvc/command/gc.py | 44 ++++++++++++++++++++++------------------- dvc/command/traverse.py | 3 +++ dvc/path/data_item.py | 13 ------------ 4 files changed, 27 insertions(+), 36 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index a5344e07b..320d7a422 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -245,9 +245,6 @@ def parse_args(argv=None): 'gc', parents=[parent_parser], help='Collect garbage') - gc_parser.add_argument('target', - nargs='*', - help='Target to remove - file or directory.') gc_parser.add_argument('-l', '--keep-in-cloud', action='store_true', diff --git a/dvc/command/gc.py b/dvc/command/gc.py index c303fadaf..88142e264 100644 --- a/dvc/command/gc.py +++ b/dvc/command/gc.py @@ -1,41 +1,45 @@ import os +from dvc.command.base import DvcLock from dvc.command.traverse import Traverse from dvc.logger import Logger - +from dvc.path.data_item import DataItemError class CmdGC(Traverse): def __init__(self, settings): super(CmdGC, self).__init__(settings, "garbage collect", do_not_start_from_root=False) + self.clist = [] + + def run(self): + with DvcLock(self.is_locker, self.git): + if not self._traverse(self.git.git_dir_abs): + Logger.error('Failed to collect used cache') + return 1 + + for cache in os.listdir(self.settings.config.cache_dir): + fname = os.path.join(self.settings.config.cache_dir, cache) + if fname in self.clist: + continue + os.remove(fname) + self._remove_cloud_cache(self.settings.path_factory.data_item('.empty', fname)) + Logger.info('Cache \'{}\' was removed'.format(fname)) + + return 0 def process_file(self, target): Logger.debug(u'[Cmd-GC] GC file {}.'.format(target)) - data_item = self._get_data_item(target) - - for cache_data_item in data_item.get_all_caches(): - if cache_data_item.cache.relative != data_item.resolved_cache.relative: - os.remove(cache_data_item.cache.relative) - Logger.info(u'GC cache file {} was removed'.format(cache_data_item.cache.relative)) - self._remove_cloud_cache(cache_data_item) + try: + data_item = self.settings.path_factory.existing_data_item(target) + except DataItemError: + return + self.clist.append(data_item.cache.relative) Logger.debug(u'[Cmd-GC] GC data item {}. Success.'.format(data_item.data.relative)) - pass def is_recursive(self): return self.parsed_args.recursive - # Renaming - def gc_dir(self, target): - Logger.debug(u'[Cmd-GC] GC dir {}.'.format(target)) - return self._traverse_dir(target) - - def gc_file(self, target): - self.process_file(target) - - def gc_all(self): - return self._traverse_all() - @property def no_git_actions(self): return True diff --git a/dvc/command/traverse.py b/dvc/command/traverse.py index 2a29ab796..b4be15839 100644 --- a/dvc/command/traverse.py +++ b/dvc/command/traverse.py @@ -53,6 +53,9 @@ class Traverse(CmdBase): return False def _traverse_dir(self, target): + if target == self.settings.config.CONFIG_DIR: + return + for f in os.listdir(target): file = os.path.join(target, f) if os.path.isdir(file): diff --git a/dvc/path/data_item.py b/dvc/path/data_item.py index 1620868ed..de93fe5ff 100644 --- a/dvc/path/data_item.py +++ b/dvc/path/data_item.py @@ -91,19 +91,6 @@ class DataItem(object): cache_file = os.path.join(cache_dir, file_name) return Path(cache_file, self._git) - def get_all_caches(self): - result = [] - - suffix_len = self._git.COMMIT_LEN + len(self.CACHE_FILE_SEP) - cache_prefix = os.path.basename(self.resolved_cache.relative[:-suffix_len]) - - for cache_file in os.listdir(self.resolved_cache.dirname): - if cache_file[:-suffix_len] == cache_prefix: - data_item = self.copy(os.path.join(self.resolved_cache.dirname, cache_file)) - result.append(data_item) - - return result - @cached_property def resolved_cache(self): resolved_cache = os.path.realpath(self._data.relative) From 9ae4c289eb4a969589e481b55e880ae314dab39a Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 29 Oct 2017 21:21:51 +0200 Subject: [PATCH 04/15] functest: fix test-init Signed-off-by: Ruslan Kuprieiev --- functest/common.sh | 2 +- functest/test-init.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/functest/common.sh b/functest/common.sh index e2aaafec3..de24a6ae0 100644 --- a/functest/common.sh +++ b/functest/common.sh @@ -106,7 +106,7 @@ function dvc_create_repo() { dvc init - mkdir data/xml + mkdir -p data/xml dvc import $DATA_CACHE/* data/xml } diff --git a/functest/test-init.sh b/functest/test-init.sh index b3a749a91..2fb773e54 100755 --- a/functest/test-init.sh +++ b/functest/test-init.sh @@ -8,7 +8,7 @@ dvc_create_git_repo dvc init -DIRS="data .dvc/cache .dvc/state" +DIRS=".dvc/cache .dvc/state" FILES=".dvc/config" dvc_check_dirs $DIRS From fb377b8506e9ee52792659ba988906a82ee21b93 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 29 Oct 2017 22:37:34 +0200 Subject: [PATCH 05/15] dvc: add 'dvc checkout' Fixes #225 Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 8 ++++++++ dvc/path/factory.py | 14 ++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/dvc/cli.py b/dvc/cli.py index 320d7a422..c5ef69039 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -17,6 +17,7 @@ from dvc.command.instance_create import CmdInstanceCreate from dvc.command.config import CmdConfig from dvc.command.show_pipeline import CmdShowPipeline from dvc.command.merge import CmdMerge +from dvc.command.checkout import CmdCheckout from dvc import VERSION @@ -412,6 +413,13 @@ def parse_args(argv=None): help='Merge') merge_parser.set_defaults(func=CmdMerge) + # Checkout + checkout_parser = subparsers.add_parser( + 'checkout', + parents=[parent_parser], + help='Checkout') + checkout_parser.set_defaults(func=CmdCheckout) + if isinstance(argv, str): argv = argv.split() diff --git a/dvc/path/factory.py b/dvc/path/factory.py index 1ca938cc5..f8ed78f2e 100644 --- a/dvc/path/factory.py +++ b/dvc/path/factory.py @@ -48,3 +48,17 @@ class PathFactory(object): externally_created_files.append(file) return result, externally_created_files + + def all_existing_data_items(self): + items = [] + states = [] + + for root, dirs, files in os.walk(self._config.state_dir): + for fname in files: + states.append(os.path.join(root, fname)) + + for state in states: + data = os.path.relpath(state, self._config.state_dir)[:-len(DataItem.STATE_FILE_SUFFIX)] + items.append(self.existing_data_item_from_dvc_path(data)) + + return items From bdea4ba2f095a782b2480de4fbaf0526aabed110 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 29 Oct 2017 22:52:48 +0200 Subject: [PATCH 06/15] dvc: fix 'dvc merge' command Signed-off-by: Ruslan Kuprieiev --- dvc/command/merge.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/dvc/command/merge.py b/dvc/command/merge.py index f730aa784..35795bd0d 100644 --- a/dvc/command/merge.py +++ b/dvc/command/merge.py @@ -1,8 +1,11 @@ +import os import json from dvc.command.base import CmdBase, DvcLock from dvc.logger import Logger from dvc.state_file import StateFile +from dvc.path.data_item import DataItem +from dvc.system import System class CmdMerge(CmdBase): def __init__(self, settings): @@ -13,9 +16,18 @@ class CmdMerge(CmdBase): Logger.info('Restored original data after merge:') Logger.info(' {}'.format(item.data.relative)) + def collect_data(self): + dlist = [] + flist = self.git.get_last_merge_changed_files() + for fname in flist: + if fname.startswith(self.settings.config.state_dir) and fname.endswith(DataItem.STATE_FILE_SUFFIX): + data = os.path.relpath(fname, self.settings.config.state_dir)[:-len(DataItem.STATE_FILE_SUFFIX)] + dlist.append(data) + return dlist + def collect_targets(self): targets = [] - flist = self.git.get_last_merge_changed_files() + flist = self.collect_data() items = self.settings.path_factory.to_data_items(flist)[0] for item in items: @@ -35,8 +47,15 @@ class CmdMerge(CmdBase): def checkout_targets(self, targets): data = [] for item in targets: - self.git.checkout_file_before_last_merge(item.data.relative) self.git.checkout_file_before_last_merge(item.state.relative) + + # Reload data item, so we can get restored cache + new_item = self.settings.path_factory.data_item(item.data.relative) + if os.path.isfile(new_item.data.relative): + os.remove(new_item.data.relative) + + System.hardlink(new_item.cache.relative, new_item.data.relative) + data.append(item.data.relative) msg = 'DVC merge files: {}'.format(' '.join(data)) From 90641c755a84b6f236cdd588cfa8f227dc158e68 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 29 Oct 2017 23:25:42 +0200 Subject: [PATCH 07/15] functest: fix merge test Signed-off-by: Ruslan Kuprieiev --- functest/common.sh | 8 ++++++++ functest/test-merge.sh | 8 ++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/functest/common.sh b/functest/common.sh index de24a6ae0..694d038d5 100644 --- a/functest/common.sh +++ b/functest/common.sh @@ -22,6 +22,14 @@ TEST_REPO_REGION='us-east-2' TEST_REPO_GCP='dvc-test/myrepo' TEST_REPO_GCP_PROJECT='dvc-project' +function dvc_md5() { + if [ "$(uname)" == "Darwin" ]; then + md5 -r $@ + else + md5sum $@ + fi +} + function dvc_print() { echo -e "$1" } diff --git a/functest/test-merge.sh b/functest/test-merge.sh index 5d6399af8..2afbe4153 100755 --- a/functest/test-merge.sh +++ b/functest/test-merge.sh @@ -18,7 +18,7 @@ git commit -s -m"add code" dvc import https://s3-us-west-2.amazonaws.com/dvc-share/so/50K/Posts.xml.tgz data/data -ORIG_DATA=$(readlink -- data/data) +ORIG_DATA=$(dvc_md5 data/data) dvc run ./code/code1.sh data/data data/data1 dvc run ./code/code2.sh data/data1 data/data2 @@ -30,7 +30,7 @@ git commit -am 'Set Target' # Create new branch to work on small data set git checkout -b input_10K -dvc remove data/data +dvc remove -c -l data/data dvc import https://s3-us-west-2.amazonaws.com/dvc-share/so/10K/Posts.xml.tgz data/data dvc repro @@ -46,10 +46,10 @@ git merge input_10K dvc merge # Verify that data and respective state file are restored properly -if [ "$(readlink -- "data/data")" != "$ORIG_DATA" ]; then +if [ "$(dvc_md5 data/data)" != "$ORIG_DATA" ]; then dvc_fail fi -grep $(basename "$ORIG_DATA") .dvc/state/data.state || dvc_fail +grep $(echo "$ORIG_DATA" | cut -d " " -f1) .dvc/state/data/data.state || dvc_fail dvc_pass From 72b691b7f11c2b2766088d8855e1bc1d574a5252 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 29 Oct 2017 23:32:41 +0200 Subject: [PATCH 08/15] dvc: add 'dvc checkout' Signed-off-by: Ruslan Kuprieiev --- dvc/command/checkout.py | 43 +++++++++++++++++++++++++++++++++++++++ functest/run.sh | 1 + functest/test-checkout.sh | 14 +++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 dvc/command/checkout.py create mode 100755 functest/test-checkout.sh diff --git a/dvc/command/checkout.py b/dvc/command/checkout.py new file mode 100644 index 000000000..153a6b4cc --- /dev/null +++ b/dvc/command/checkout.py @@ -0,0 +1,43 @@ +import os +from stat import ST_INO, ST_DEV + +from dvc.command.base import CmdBase, DvcLock +from dvc.logger import Logger +from dvc.system import System + +class CmdCheckout(CmdBase): + def __init__(self, settings): + super(CmdCheckout, self).__init__(settings) + + @staticmethod + def cache_ok(item): + data = item.data.relative + cache = item.cache.relative + + if not os.path.isfile(data) or not os.path.isfile(cache): + return False + + data_st = os.stat(data) + cache_st = os.stat(cache) + + if (data_st[ST_INO], data_st[ST_DEV]) != (cache_st[ST_INO], cache_st[ST_DEV]): + return False + + return True + + def checkout(self, items): + for item in items: + if self.cache_ok(item): + continue + + if os.path.isfile(item.data.relative): + os.remove(item.data.relative) + + System.hardlink(item.cache.relative, item.data.relative) + Logger.info('Checkout \'{}\''.format(item.data.relative)) + + def run(self): + with DvcLock(self.is_locker, self.git): + items = self.settings.path_factory.all_existing_data_items() + self.checkout(items) + return 0 diff --git a/functest/run.sh b/functest/run.sh index 0cc9ac5ee..0f235d26c 100755 --- a/functest/run.sh +++ b/functest/run.sh @@ -9,6 +9,7 @@ TESTS+=" test-import.sh" TESTS+=" test-sync.sh" TESTS+=" test-repro-code.sh" TESTS+=" test-merge.sh" +TESTS+=" test-checkout.sh" for t in $TESTS; do rm -rf $TEST_REPO diff --git a/functest/test-checkout.sh b/functest/test-checkout.sh new file mode 100755 index 000000000..4609cc202 --- /dev/null +++ b/functest/test-checkout.sh @@ -0,0 +1,14 @@ +#!/bin/bash + +set -e + +source common.sh + +ITEM=data/xml/Badges.xml + +dvc_create_repo + +rm -f $ITEM +dvc checkout +dvc_check_files $ITEM +dvc_pass From 664ec5dfa46a3195e46585d1e8007bcd2c287c9e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 30 Oct 2017 02:52:52 +0200 Subject: [PATCH 09/15] tests: remove gc unit test After moving to md5 for cache files, gc logic became very straightforward, and this unit test became irrelevant. We still have functional test for gc though, so it is not like we're leaving gc untested. Probably worth it to write simple unit test for it later. Signed-off-by: Ruslan Kuprieiev --- tests/test_cmd_gc.py | 138 ------------------------------------------- 1 file changed, 138 deletions(-) delete mode 100644 tests/test_cmd_gc.py diff --git a/tests/test_cmd_gc.py b/tests/test_cmd_gc.py deleted file mode 100644 index 5d8d79c2e..000000000 --- a/tests/test_cmd_gc.py +++ /dev/null @@ -1,138 +0,0 @@ -import glob -import os - -from dvc.command.gc import CmdGC -from dvc.config import ConfigI -from dvc.git_wrapper import GitWrapperI -from dvc.path.data_item import DataItem -from dvc.path.factory import PathFactory -from dvc.settings import Settings -from dvc.system import System -from tests.basic_env import BasicEnvironment - -class TestCmdDataRemove(BasicEnvironment): - def setUp(self): - self._test_dir = os.path.join(os.path.sep, 'tmp', 'dvc_unit_test') - curr_dir = None - commit = 'abc1234' - - BasicEnvironment.init_environment(self, self._test_dir, curr_dir) - - self._commit = commit - - self._git = GitWrapperI(git_dir=self._test_git_dir, commit=self._commit) - self._config = ConfigI('data', 'cache', 'state') - self.path_factory = PathFactory(self._git, self._config) - self.settings = Settings('gc', self._git, self._config) - - self.dir1 = 'dir1' - - self.create_dirs(self.dir1) - - def test_single_file(self): - os.chdir(self._test_git_dir) - - self.crate_data_item_with_five_caches('', 'file1.txt') - self.assertEqual(5, self.cache_file_nums('file1*')) - - cmd = CmdGC(self.settings) - os.chdir(self._test_git_dir) - - cmd.gc_file(os.path.join('data', 'file1.txt')) - self.assertEqual(1, self.cache_file_nums('file1*')) - - def test_all(self): - os.chdir(self._test_git_dir) - - self.crate_data_item_with_five_caches('', 'file1.txt') - self.crate_data_item_with_five_caches('', 'file2.txt') - self.crate_data_item_with_five_caches(self.dir1, 'file3.txt') - - self.assertEqual(5, self.cache_file_nums('file1*')) - self.assertEqual(5, self.cache_file_nums('file2*')) - self.assertEqual(5, self.cache_file_nums(os.path.join(self.dir1, 'file3*'))) - - self.settings.parse_args('gc --no-git-actions data') - cmd = CmdGC(self.settings) - os.chdir(self._test_git_dir) - - cmd.gc_all() - - self.assertEqual(1, self.cache_file_nums('file1*')) - self.assertEqual(1, self.cache_file_nums('file2*')) - self.assertEqual(1, self.cache_file_nums(os.path.join(self.dir1, 'file3*'))) - - def test_empty(self): - os.chdir(self._test_git_dir) - - self.crate_data_item_with_five_caches('', 'file1.txt') - self.crate_data_item_with_five_caches('', 'file2.txt') - self.assertEqual(5, self.cache_file_nums('file1*')) - self.assertEqual(5, self.cache_file_nums('file2*')) - - self.settings.parse_args('gc --no-git-actions') - cmd = CmdGC(self.settings) - os.chdir(self._test_git_dir) - - cmd.gc_all() - - self.assertEqual(5, self.cache_file_nums('file1*')) - self.assertEqual(5, self.cache_file_nums('file2*')) - - def create_dirs(self, dir): - os.mkdir(os.path.join(self._test_git_dir, 'data', dir)) - os.mkdir(os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir)) - os.mkdir(os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, dir)) - - def cache_file_nums(self, pattern): - os.chdir(os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR)) - files = [] - for file in glob.glob(pattern): - files.append(file) - return len(files) - - def crate_data_item_with_five_caches(self, dir, data_file, content='random text'): - os.chdir(self._test_git_dir) - - file_result = os.path.join('data', dir, data_file) - - state_result = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, dir, data_file + DataItem.STATE_FILE_SUFFIX) - self.create_content_file(state_result, 'state content') - - file_prefix = data_file + DataItem.CACHE_FILE_SEP - cache_result = os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir, file_prefix + self._commit) - self.create_content_file(cache_result, content) - relevant_dir = self.relevant_dir(os.path.join(dir, data_file)) - cache_file = os.path.join(relevant_dir, cache_result) - - d = os.path.join(self._test_git_dir, 'data', dir) - os.chdir(d) - print('---- CREATE SL {}: {} --> {}'.format(d, data_file, cache_file)) - System.symlink(cache_file, data_file) - os.chdir(self._test_git_dir) - - data_item = self.settings.path_factory.existing_data_item(file_result) - print('*DATA ITEM {}: {} {}'.format(file_result, data_item.data.relative, data_item.cache.relative)) - - print('----> {} {} {} {}'.format(dir, data_file, cache_file, file_result)) - - # Additional cache files - self.create_content_file(os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir, file_prefix + 'aaaaaaa')) - self.create_content_file(os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir, file_prefix + '1111111')) - self.create_content_file(os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir, file_prefix + '5555555')) - self.create_content_file(os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir, file_prefix + '123abff')) - return - - @staticmethod - def create_content_file(file, content='some test'): - fd = open(file, 'w+') - fd.write(content) - fd.close() - - @staticmethod - def relevant_dir(data_file): - deepth = data_file.count(os.path.sep) - relevant_path = '..' - for i in range(deepth): - relevant_path = os.path.join(relevant_path, '..') - return relevant_path From afff00048bcedbb01eb600d8db236f879820b0a7 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 30 Oct 2017 02:57:06 +0200 Subject: [PATCH 10/15] tests: utils: remove sleep() No real need to sleep that much in the unit test, as it really slows down everything. Signed-off-by: Ruslan Kuprieiev --- tests/test_utils.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/test_utils.py b/tests/test_utils.py index ba2862a34..549ae9e86 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,8 +1,6 @@ import os import shutil import filecmp -from random import randint -from time import sleep from dvc import utils from tests.basic_env import DirHierarchyEnvironment @@ -48,10 +46,8 @@ class TestExecutor(DirHierarchyEnvironment): def test_map_progress(self): def f(target): - sleep(randint(1,2)) with open(target, 'w+') as o: o.write(target) - sleep(randint(1,2)) targets = ['map{}'.format(i) for i in range(1, 10)] n_threads = [1, 10, 20] From 52b47a25b4c66a41ce2428c9167381b8f67a7dc7 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 30 Oct 2017 03:57:25 +0200 Subject: [PATCH 11/15] test: fix unit tests Signed-off-by: Ruslan Kuprieiev --- tests/basic_env.py | 20 ++------ tests/test_cmd_remove.py | 4 +- tests/test_cmd_run.py | 14 +----- tests/test_data_file_obj.py | 86 ++++++++------------------------- tests/test_import.py | 3 +- tests/test_repository_change.py | 15 +----- 6 files changed, 32 insertions(+), 110 deletions(-) diff --git a/tests/basic_env.py b/tests/basic_env.py index b5645b429..ebfdab9fd 100644 --- a/tests/basic_env.py +++ b/tests/basic_env.py @@ -35,7 +35,7 @@ class BasicEnvironment(TestCase): data_dir = os.path.join(self._test_git_dir, 'data') cache_dir = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR) - state_dir = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR) + state_dir = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'data') os.makedirs(data_dir) os.makedirs(cache_dir) @@ -103,7 +103,7 @@ class DirHierarchyEnvironment(BasicEnvironment): def crate_data_item(self, data_file, cache_file=True, content='random text'): file_result = os.path.join('data', data_file) - state_result = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, data_file + DataItem.STATE_FILE_SUFFIX) + state_result = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, file_result + DataItem.STATE_FILE_SUFFIX) dummy_md5 = os.path.basename(data_file) + DataItem.CACHE_FILE_SEP + self._commit self.create_content_file(state_result, '{"Md5" : "' + dummy_md5 + '", "Command" : "run", "Cwd" : "dir"}') @@ -112,23 +112,13 @@ class DirHierarchyEnvironment(BasicEnvironment): cache_result = os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dummy_md5) self.create_content_file(cache_result, content) - relevant_dir = self.relevant_dir(data_file) - System.symlink(os.path.join(relevant_dir, cache_result), file_result) + System.hardlink(cache_result, file_result) else: cache_result = None self.create_content_file(file_result, content) return file_result, cache_result, state_result - @staticmethod - def relevant_dir(data_file): - deepth = data_file.count(os.path.sep) - relevant_path = '..' - for i in range(deepth): - relevant_path = os.path.join(relevant_path, '..') - return relevant_path - def create_dirs(self, dir): - os.mkdir(os.path.join('data', dir)) - os.mkdir(os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dir)) - os.mkdir(os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, dir)) + os.makedirs(os.path.join('data', dir)) + os.makedirs(os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'data', dir)) diff --git a/tests/test_cmd_remove.py b/tests/test_cmd_remove.py index 168f3a8c6..bbd24454d 100644 --- a/tests/test_cmd_remove.py +++ b/tests/test_cmd_remove.py @@ -139,8 +139,8 @@ class TestRemoveEndToEndWithAnError(DirHierarchyEnvironment): DirHierarchyEnvironment.init_environment(self) def test(self): - dir11_full = os.path.join(self._config.data_dir, self.dir11) - dir2_full = os.path.join(self._config.data_dir, self.dir2) + dir11_full = os.path.join('data', self.dir11) + dir2_full = os.path.join('data', self.dir2) self.settings.parse_args('remove --no-git-actions --keep-in-cloud --recursive {} {}'.format(dir11_full, self.file5)) cmd = CmdRemove(self.settings) diff --git a/tests/test_cmd_run.py b/tests/test_cmd_run.py index fb6fc249b..8c2772390 100644 --- a/tests/test_cmd_run.py +++ b/tests/test_cmd_run.py @@ -55,16 +55,6 @@ class RunBasicTest(TestCase): os.chdir(self._old_curr_dir_abs) -class TestRunOutsideData(RunBasicTest): - def test(self): - self.settings._args = [] - cmd_run = CmdRun(self.settings) - with self.assertRaises(RunError): - cmd_run.run_command(['echo', 'test'], [], [], shell=True, - stdout='file1', stderr='file2') - pass - - class RunTwoFilesBase(RunBasicTest): def setUp(self): super(RunTwoFilesBase, self).setUp() @@ -82,8 +72,8 @@ class RunTwoFilesBase(RunBasicTest): stdout=self.file_name1, stderr=self.file_name2) - self.state_file_name1 = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'file1' + DataItem.STATE_FILE_SUFFIX) - self.state_file_name2 = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'file2' + DataItem.STATE_FILE_SUFFIX) + self.state_file_name1 = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, self.file_name1 + DataItem.STATE_FILE_SUFFIX) + self.state_file_name2 = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, self.file_name2 + DataItem.STATE_FILE_SUFFIX) self.state_file1 = None self.state_file2 = None diff --git a/tests/test_data_file_obj.py b/tests/test_data_file_obj.py index 848dd3672..cc7ea821a 100644 --- a/tests/test_data_file_obj.py +++ b/tests/test_data_file_obj.py @@ -2,7 +2,7 @@ import os from dvc.config import ConfigI from dvc.git_wrapper import GitWrapperI -from dvc.path.data_item import NotInDataDirError, DataItemError, DataItem +from dvc.path.data_item import DataDirError, DataItemError, DataItem from dvc.path.factory import PathFactory from dvc.system import System from tests.basic_env import BasicEnvironment @@ -13,7 +13,7 @@ class TestDataFileObjBasic(BasicEnvironment): BasicEnvironment.init_environment(self, test_dir=os.path.join(os.path.sep, 'tmp', 'ntx_unit_test')) git = GitWrapperI(git_dir=self._test_git_dir, commit='ad45ba8') - config = ConfigI('data') + config = ConfigI() file = os.path.join('data', 'file.txt') self.cache_file_full_path = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, 'file.txt_ad45ba8') @@ -28,13 +28,9 @@ class TestDataFileObjBasic(BasicEnvironment): self.assertEqual(self._data_path.cache.abs, self.cache_file_full_path) def test_state_file(self): - state_file_full_path = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'file.txt.state') + state_file_full_path = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'data', 'file.txt.state') self.assertEqual(self._data_path.state.abs, state_file_full_path) - def test_data_dvs_short(self): - self.assertEqual(self._data_path.data_dvc_short, 'file.txt') - pass - class TestDataItemWithGivenCache(BasicEnvironment): def setUp(self): @@ -53,9 +49,6 @@ class TestDataItemWithGivenCache(BasicEnvironment): self.assertEqual(self._data_item.data.relative, self._file) self.assertEqual(self._data_item.cache.relative, self._cache) - def test_resolve_unexist_symlink(self): - self.assertEqual(self._data_item.resolved_cache.relative, self._file) - class TestDeepDataItemWithGivenCache(BasicEnvironment): def setUp(self): @@ -80,18 +73,22 @@ class TestPathFactory(BasicEnvironment): BasicEnvironment.init_environment(self, test_dir=os.path.join(os.path.sep, 'tmp', 'ntx_unit_test')) git = GitWrapperI(git_dir=self._test_git_dir, commit='ad45ba8') - config = ConfigI('data') + config = ConfigI() self.path_factory = PathFactory(git, config) self.data_file = os.path.join('data', 'file.txt') - self.cache_file = os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, 'fsymlinc.txt') + dummy_md5 = 'fsymlinc.txt' + self.cache_file = os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, dummy_md5) + self.state_file = os.path.join(ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'data', 'file.txt' + DataItem.STATE_FILE_SUFFIX) - fd = open(self.cache_file, 'w+') - fd.write('some text') - fd.close() + with open(self.cache_file, 'w+') as fd: + fd.write('some text') + + with open(self.state_file, 'w+') as fd: + fd.write('{"Md5" : "' + dummy_md5 + '", "Command" : "run", "Cwd" : "dir"}') os.chdir('data') - System.symlink(os.path.join('..', self.cache_file), 'file.txt') + System.hardlink(os.path.join('..', self.cache_file), 'file.txt') os.chdir('..') pass @@ -104,17 +101,17 @@ class TestPathFactory(BasicEnvironment): self.assertEqual(data_path_indirect.data.dvc, self.data_file) pass - def test_data_symlink_factory(self): + def test_data_hardlink_factory(self): data_path = self.path_factory.existing_data_item(self.data_file) self.assertEqual(data_path.cache.dvc, self.cache_file) pass - def test_data_symlink_factory_cache(self): + def test_data_hardlink_factory_cache(self): data_path = self.path_factory.existing_data_item(self.data_file) self.assertEqual(data_path.data.dvc, self.data_file) pass - def test_data_symlink_factory_exception(self): + def test_data_hardlink_factory_exception(self): with self.assertRaises(DataItemError): self.path_factory.existing_data_item(self.cache_file) pass @@ -126,25 +123,6 @@ class TestPathFactory(BasicEnvironment): self.assertEqual(path.relative, file) self.assertTrue(path.abs.endswith(file)) - def test_to_data_path(self): - exclude_file = os.path.join(ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, 'file2') - data_path_file1 = os.path.join('data', 'file1') - data_path_file2 = os.path.join('data', 'file2') - files = [ - data_path_file1, - exclude_file, - data_path_file2 - ] - - data_path_list, exclude_file_list = self.path_factory.to_data_items(files) - - self.assertEqual(len(data_path_list), 2) - self.assertEqual(len(exclude_file_list), 1) - - self.assertEqual(exclude_file_list[0], exclude_file) - data_path_set = set(x.data.dvc for x in data_path_list) - self.assertEqual(data_path_set, {data_path_file1, data_path_file2}) - class TestDataPathInDataDir(BasicEnvironment): def setUp(self): @@ -152,7 +130,7 @@ class TestDataPathInDataDir(BasicEnvironment): git = GitWrapperI(git_dir=self._test_git_dir, commit='eeeff8f') self.data_dir = 'da' - config = ConfigI(self.data_dir) + config = ConfigI() self.path_factory = PathFactory(git, config) deep_path = os.path.join(self.data_dir, 'dir1', 'd2', 'file.txt') @@ -168,18 +146,9 @@ class TestDataPathInDataDir(BasicEnvironment): self.assertEqual(self.data_path.cache.abs, self.cache_file) def test_state_file(self): - target = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'dir1', 'd2', 'file.txt.state') + target = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'da', 'dir1', 'd2', 'file.txt.state') self.assertEqual(self.data_path.state.abs, target) - def test_symlink(self): - expected = os.path.join('..', '..', '..', ConfigI.CONFIG_DIR, ConfigI.CACHE_DIR, 'dir1', 'd2', 'file.txt_eeeff8f') - self.assertEqual(self.data_path.symlink_file, expected) - - def test_data_dir(self): - data_path = self.path_factory.data_item(self.data_dir) - self.assertEqual(data_path.data.dvc, self.data_dir) - self.assertEqual(data_path.data_dvc_short, '') - class TestDataFileObjLongPath(BasicEnvironment): def setUp(self): @@ -205,24 +174,9 @@ class TestDataFileObjLongPath(BasicEnvironment): self.assertEqual(self.data_path.cache.abs, self.cache_file) def test_state_file(self): - target = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'file1.txt.state') + target = os.path.join(self._test_git_dir, ConfigI.CONFIG_DIR, ConfigI.STATE_DIR, 'data', 'file1.txt.state') self.assertEqual(self.data_path.state.abs, target) - def test_file_name_only(self): - with self.assertRaises(NotInDataDirError): - self.path_factory.data_item('file.txt') - pass - - def test_relative_git_dir(self): - with self.assertRaises(NotInDataDirError): - self.path_factory.data_item('data/file.txt') - pass - - def test_relative_path_error(self): - with self.assertRaises(NotInDataDirError): - self.path_factory.data_item('../data/file.txt') - pass - class RunOutsideGitRepoTest(BasicEnvironment): def setUp(self): @@ -237,6 +191,6 @@ class RunOutsideGitRepoTest(BasicEnvironment): config = ConfigI('data') path_factory = PathFactory(git, config) - with self.assertRaises(NotInDataDirError): + with self.assertRaises(DataDirError): path_factory.data_item('file.txt') pass diff --git a/tests/test_import.py b/tests/test_import.py index 96b24fcc6..8cec2ef39 100644 --- a/tests/test_import.py +++ b/tests/test_import.py @@ -1,5 +1,6 @@ import os import tempfile +import filecmp from copy import copy from dvc.command.import_file import CmdImportFile, ImportFileError @@ -26,7 +27,7 @@ class TestCmdRemove(DirHierarchyEnvironment): data_item = self.path_factory.existing_data_item(dvc_name) self.assertTrue(os.path.exists(data_item.data.relative)) - self.assertTrue(os.path.islink(data_item.data.relative)) + self.assertTrue(filecmp.cmp(data_item.data.relative, data_item.cache.relative)) self.assertEqual(open(data_item.data.relative).read(), content) self.assertTrue(os.path.exists(data_item.cache.relative)) diff --git a/tests/test_repository_change.py b/tests/test_repository_change.py index 2425464cb..cbce53338 100644 --- a/tests/test_repository_change.py +++ b/tests/test_repository_change.py @@ -26,7 +26,7 @@ class BasicTestRepositoryChange(TestCase): subprocess.Popen(['git', 'init'], stdout=self._devnull, stderr=None).wait() self.git = GitWrapperI(self.test_dir) - self.config = ConfigI('data', 'cache', 'state') + self.config = ConfigI() self.path_factory = PathFactory(self.git, self.config) self.settings = Settings([], self.git, self.config) @@ -96,16 +96,3 @@ class TestRepositoryChangeDeepInDirectory(BasicTestRepositoryChange): self.assertEqual([x.data.dvc for x in change.new_data_items], [deep_file]) -class TestRepositoryChangeExternallyCreated(BasicTestRepositoryChange): - def test(self): - not_in_data_dir_file = 'file1.txt' - self.create_file(not_in_data_dir_file) - - change = RepositoryChange(['echo', 'textsd'], - self.settings, - not_in_data_dir_file, - None, - shell=True) - - self.assertEqual([System.realpath(not_in_data_dir_file)], - change.externally_created_files) From 299e4e2ae1cca4e9bde2cafc22cf57971c548ccb Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 30 Oct 2017 04:54:23 +0200 Subject: [PATCH 12/15] checkout: use os.path.samefile instead of stat Signed-off-by: Ruslan Kuprieiev --- dvc/command/checkout.py | 6 +----- dvc/system.py | 7 +++++++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/dvc/command/checkout.py b/dvc/command/checkout.py index 153a6b4cc..b135642ab 100644 --- a/dvc/command/checkout.py +++ b/dvc/command/checkout.py @@ -1,5 +1,4 @@ import os -from stat import ST_INO, ST_DEV from dvc.command.base import CmdBase, DvcLock from dvc.logger import Logger @@ -17,10 +16,7 @@ class CmdCheckout(CmdBase): if not os.path.isfile(data) or not os.path.isfile(cache): return False - data_st = os.stat(data) - cache_st = os.stat(cache) - - if (data_st[ST_INO], data_st[ST_DEV]) != (cache_st[ST_INO], cache_st[ST_DEV]): + if not System.samefile(data, cache): return False return True diff --git a/dvc/system.py b/dvc/system.py index bb4d41acc..55f61ccc9 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -23,6 +23,13 @@ class System(object): raise Exception('Not Implemented Yet') + @staticmethod + def samefile(path1, path2): + if System.is_unix(): + return os.path.samefile(path1, path2) + + raise Exception('Not Implemented Yet') + @staticmethod def realpath(path): # It is definitely not the best way to check a symlink. From 2dcb9ac37baaebe521dcad691c99989ddf6eecb6 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 31 Oct 2017 13:17:20 +0200 Subject: [PATCH 13/15] dvc: factory: check if file inside state dir has proper suffix Signed-off-by: Ruslan Kuprieiev --- dvc/path/factory.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dvc/path/factory.py b/dvc/path/factory.py index f8ed78f2e..efae5bf99 100644 --- a/dvc/path/factory.py +++ b/dvc/path/factory.py @@ -55,7 +55,13 @@ class PathFactory(object): for root, dirs, files in os.walk(self._config.state_dir): for fname in files: - states.append(os.path.join(root, fname)) + path = os.path.join(root, fname) + + if not fname.endswith(DataItem.STATE_FILE_SUFFIX): + Logger.warn('Found file \'{}\' without state suffix inside state dir'.format(path)) + continue + + states.append(path) for state in states: data = os.path.relpath(state, self._config.state_dir)[:-len(DataItem.STATE_FILE_SUFFIX)] From 54c4ce1b491c649472d5737e4d812e72fd68cda1 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 31 Oct 2017 14:28:47 +0200 Subject: [PATCH 14/15] functest: fix tutorial Signed-off-by: Ruslan Kuprieiev --- functest/tutorial_end_to_end.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/functest/tutorial_end_to_end.sh b/functest/tutorial_end_to_end.sh index e209023d6..f3d732845 100755 --- a/functest/tutorial_end_to_end.sh +++ b/functest/tutorial_end_to_end.sh @@ -4,8 +4,10 @@ set -e # 1. First ML model +rm -rf myrepo mkdir myrepo cd myrepo +mkdir data mkdir code wget -nv -P code/ https://s3-us-west-2.amazonaws.com/dvc-share/so/code/featurization.py \ https://s3-us-west-2.amazonaws.com/dvc-share/so/code/evaluate.py \ From f91abed4163446f2ef5ea83b1dee4627a76041eb Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 31 Oct 2017 14:51:46 +0200 Subject: [PATCH 15/15] git: check if gitignore already has file before modifying it Signed-off-by: Ruslan Kuprieiev --- dvc/git_wrapper.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/dvc/git_wrapper.py b/dvc/git_wrapper.py index ff3a41341..e142bc8b6 100644 --- a/dvc/git_wrapper.py +++ b/dvc/git_wrapper.py @@ -96,6 +96,17 @@ class GitWrapperI(object): def commit_all_changes_and_log_status(self, message): pass + @staticmethod + def _file_contains(fname, s): + if not os.path.isfile(fname): + return False + + with open(fname, 'r') as fd: + if s in fd.read(): + return True + + return False + @staticmethod def modify_gitignore(flist): for fname in flist: @@ -104,8 +115,13 @@ class GitWrapperI(object): # git and we will not have any problems with --porcelain output for # specific changed files. gitignore = os.path.join(os.path.dirname(fname), '.gitignore') + entry = os.path.basename(fname) + + if GitWrapperI._file_contains(gitignore, entry + '\n'): + return + with open(gitignore, 'a') as fd: - fd.write('\n' + os.path.basename(fname)) + fd.write('\n' + entry) class GitWrapper(GitWrapperI):