From 3083604a9c3999f4fef64a4b0779cdf14009aa3b Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Thu, 23 Nov 2017 13:36:03 +0200 Subject: [PATCH] dvc: remove 'import' and replace it with 'add' Allows us to unify state files and drop another level of abstraction. Fixes #304 Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 25 ++----- dvc/command/add.py | 56 ++++++++++++++ dvc/command/import_file.py | 145 ------------------------------------ dvc/command/merge.py | 17 +---- dvc/command/repro.py | 35 +-------- dvc/command/run.py | 3 +- dvc/state_file.py | 22 +----- functest/common.sh | 3 +- functest/run.sh | 2 +- functest/test-add.sh | 13 ++++ functest/test-import.sh | 24 ------ functest/test-merge.sh | 6 +- functest/test-repro-code.sh | 3 +- 13 files changed, 94 insertions(+), 260 deletions(-) create mode 100644 dvc/command/add.py delete mode 100644 dvc/command/import_file.py create mode 100755 functest/test-add.sh delete mode 100755 functest/test-import.sh diff --git a/dvc/cli.py b/dvc/cli.py index 9db4f3841..83f2a0458 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -12,7 +12,7 @@ from dvc.command.repro import CmdRepro from dvc.command.data_sync import CmdDataPush, CmdDataPull, CmdDataStatus from dvc.command.lock import CmdLock from dvc.command.gc import CmdGC -from dvc.command.import_file import CmdImportFile +from dvc.command.add import CmdAdd from dvc.command.show_workflow import CmdShowWorkflow from dvc.command.instance_create import CmdInstanceCreate from dvc.command.config import CmdConfig @@ -210,29 +210,16 @@ def parse_args(argv=None): help='Do not remove data from cache.') remove_parser.set_defaults(func=CmdRemove) - # Import + # Add import_parser = subparsers.add_parser( - 'import', + 'add', parents=[parent_parser], - help='Import file to data directory.') + help='Add files/directories to dvc') import_parser.add_argument( 'input', nargs='+', - help='Input file/files.') - import_parser.add_argument( - 'output', - help='Output file/directory.') - import_parser.add_argument('-l', - '--lock', - action='store_true', - default=False, - help='Lock data item - disable reproduction.') - import_parser.add_argument('-j', - '--jobs', - type=int, - default=cpu_count(), - help='Number of jobs to run simultaneously.') - import_parser.set_defaults(func=CmdImportFile) + help='Input files/directories') + import_parser.set_defaults(func=CmdAdd) # Lock lock_parser = subparsers.add_parser( diff --git a/dvc/command/add.py b/dvc/command/add.py new file mode 100644 index 000000000..54ef60d6f --- /dev/null +++ b/dvc/command/add.py @@ -0,0 +1,56 @@ +import os + +from dvc.command.base import CmdBase, DvcLock +from dvc.logger import Logger +from dvc.state_file import StateFile + +class CmdAdd(CmdBase): + def __init__(self, settings): + super(CmdAdd, self).__init__(settings) + + def collect_file(self, fname): + return [self.settings.path_factory.data_item(fname)] + + def collect_dir(self, dname): + targets = [] + for root, dirs, files in os.walk(dname): + for fname in files: + targets += self.collect_file(os.path.join(root, fname)) + return targets + + def collect_targets(self, inputs): + targets = [] + for i in inputs: + if not os.path.isdir(i): + targets += self.collect_file(i) + else: + targets += self.collect_dir(i) + return targets + + def add_files(self, targets): + for data_item in targets: + data_item.move_data_to_cache() + + def create_state_files(self, targets): + """ + Create state files for all targets. + """ + for data_item in targets: + Logger.debug('Creating state file for {}'.format(data_item.data.relative)) + + state_file = StateFile(data_item, + self.settings, + argv=[], + input_items=[], + output_files=[], + lock=True) + state_file.save() + Logger.debug('State file "{}" was created'.format(data_item.state.relative)) + + def run(self): + with DvcLock(self.is_locker, self.git): + targets = self.collect_targets(self.parsed_args.input) + self.add_files(targets) + self.create_state_files(targets) + msg = 'DVC add: {}'.format(str(self.parsed_args.input)) + self.commit_if_needed(msg) diff --git a/dvc/command/import_file.py b/dvc/command/import_file.py deleted file mode 100644 index 453f1ccf7..000000000 --- a/dvc/command/import_file.py +++ /dev/null @@ -1,145 +0,0 @@ -import os - -try: - from urlparse import urlparse -except ImportError: - # Python 3 - # pylint: disable=no-name-in-module, import-error - from urllib.parse import urlparse - -from dvc.command.common.base import CmdBase -from dvc.logger import Logger -from dvc.exceptions import DvcException -from dvc.state_file import StateFile - - -class ImportFileError(DvcException): - def __init__(self, msg): - DvcException.__init__(self, 'Import file: {}'.format(msg)) - - -class CmdImportFile(CmdBase): - def __init__(self, settings): - super(CmdImportFile, self).__init__(settings) - - def run(self): - targets = [] - output = self.parsed_args.output - self.verify_output(output, self.parsed_args.input) - - for input in self.parsed_args.input: - if not os.path.isdir(input): - targets.append((input, output)) - else: - input_dir = os.path.basename(input) - for root, dirs, files in os.walk(input): - for file in files: - filename = os.path.join(root, file) - - rel = os.path.relpath(filename, input) - out = os.path.join(output, input_dir, rel) - - out_dir = os.path.dirname(out) - if not os.path.exists(out_dir): - os.mkdir(out_dir) - - targets.append((filename, out)) - pass - self.import_files(targets, self.parsed_args.lock, self.parsed_args.jobs) - message = 'DVC import files: {} -> {}'.format(str(self.parsed_args.input), output) - self.commit_if_needed(message) - - @staticmethod - def verify_output(output, input): - if CmdImportFile.is_dir_path(output) and not os.path.isdir(output): - raise ImportFileError(u'output directory {} does not exist'.format(output)) - - if len(input) > 1 and not os.path.isdir(output): - msg = u'output {} has to be directory for multiple file import' - raise ImportFileError(msg.format(output)) - pass - - @staticmethod - def is_dir_path(output): - return len(output) > 0 and output[-1] == os.path.sep - - def import_and_commit_if_needed(self, input, output, lock=False, check_if_ready=True, is_repro=False): - if check_if_ready and not self.no_git_actions and not self.git.is_ready_to_go(): - return 1 - - self.import_file(input, output, lock) - - cmd_name = 'repro-import' if is_repro else 'import' - message = 'DVC {} file: {} {}'.format(cmd_name, input, output) - return self.commit_if_needed(message) - - def import_file(self, input, output, lock): - return self.import_files([(input, output)], lock) - - def collect_targets(self, targets): - """ - Collect input, output and data_item's into tuples - to be able to download them simulteniously later. - """ - data_targets = [] - - for target in targets: - input = target[0] - output = target[1] - if not CmdImportFile.is_url(input): - if not os.path.exists(input): - raise ImportFileError('Input file "{}" does not exist'.format(input)) - if not os.path.isfile(input): - raise ImportFileError('Input file "{}" has to be a regular file'.format(input)) - - if os.path.isdir(output): - output = os.path.join(output, os.path.basename(input)) - - data_item = self.settings.path_factory.data_item(output) - - if os.path.exists(data_item.data.relative): - raise ImportFileError('Output file "{}" already exists'.format(data_item.data.relative)) - if not os.path.isdir(os.path.dirname(data_item.data.relative)): - raise ImportFileError('Output file directory "{}" does not exists'.format( - os.path.dirname(data_item.data.relative))) - - if not os.path.exists(data_item.cache_dir_abs): - os.makedirs(data_item.cache_dir_abs) - - data_targets.append((input, data_item)) - - return data_targets - - def import_files(self, targets, lock=False, jobs=1): - data_targets = self.collect_targets(targets) - imported_targets = self.cloud.import_data(data_targets, jobs) - self.create_state_files(imported_targets, lock) - - def create_state_files(self, targets, lock): - """ - Create state files for all targets. - """ - for t in targets: - orig_target, processed_data_item = t - input, data_item = orig_target - output = data_item.data.relative - - if processed_data_item == None: - Logger.debug('Skipping creating state file for failed import {}'.format(data_item.state.relative)) - continue - - Logger.debug('Creating state file for {}'.format(data_item.data.relative)) - - state_file = StateFile(StateFile.COMMAND_IMPORT_FILE, - data_item, - self.settings, - argv=[input, output], - input_items=[], - output_files=[output], - lock=lock) - state_file.save() - Logger.debug('State file "{}" was created'.format(data_item.state.relative)) - - @staticmethod - def is_url(url): - return len(urlparse(url).scheme) != 0 diff --git a/dvc/command/merge.py b/dvc/command/merge.py index 8228baac3..4b608baa0 100644 --- a/dvc/command/merge.py +++ b/dvc/command/merge.py @@ -23,8 +23,8 @@ class CmdMerge(CmdBase): dlist = [] flist = self.git.get_last_merge_changed_files() for fname in flist: - if fname.startswith(ConfigI.STATE_DIR) and fname.endswith(DataItem.STATE_FILE_SUFFIX): - data = os.path.relpath(fname, ConfigI.STATE_DIR)[:-len(DataItem.STATE_FILE_SUFFIX)] + if fname.startswith(self.settings.config.state_dir) and fname.endswith(DataItem.CACHE_STATE_FILE_SUFFIX): + data = os.path.relpath(fname, self.settings.config.state_dir)[:-len(DataItem.CACHE_STATE_FILE_SUFFIX)] dlist.append(data) return dlist @@ -40,10 +40,8 @@ class CmdMerge(CmdBase): Logger.error('Failed to load state file for {}'.format(item.data.relative), exc_info=True) return None - if not state.command == StateFile.COMMAND_IMPORT_FILE: - continue - - targets.append(item) + if not state.argv and state.locked: + targets.append(item) return targets @@ -55,13 +53,6 @@ class CmdMerge(CmdBase): CmdCheckout.checkout([item]) -# if os.path.isfile(item.data.relative): -# os.remove(item.data.relative) -# -# System.hardlink(item.cache.relative, item.data.relative) -# -# data.append(item.data.relative) -# msg = 'DVC merge files: {}'.format(' '.join(data)) self.commit_if_needed(msg) diff --git a/dvc/command/repro.py b/dvc/command/repro.py index 14dec61f5..1da83b876 100644 --- a/dvc/command/repro.py +++ b/dvc/command/repro.py @@ -1,7 +1,7 @@ import os import copy -from dvc.command.import_file import CmdImportFile +from dvc.command.base import DvcLock from dvc.command.run import CmdRun from dvc.logger import Logger from dvc.exceptions import DvcException @@ -115,10 +115,10 @@ class ReproChange(object): raise ReproError('Error: state file "{}" cannot be loaded: {}'. format(data_item.state.relative, ex)) - if not self.state.argv: + if not self.state.argv and not self.state.locked: raise ReproError('Error: parameter {} is not defined in state file "{}"'. format(StateFile.PARAM_ARGV, data_item.state.relative)) - if len(self.state.argv) < 1: + if len(self.state.argv) < 1 and not self.state.locked: raise ReproError('Error: reproducible cmd in state file "{}" is too short'. format(self.state.file)) @@ -149,27 +149,6 @@ class ReproChange(object): msg = 'Data item {} cannot be removed before reproduction: {}' Logger.error(msg.format(output_dvc, ex)) - def reproduce_import(self): - Logger.debug('Reproducing data item {}. Re-import cmd: {}'.format( - self._data_item.data.relative, ' '.join(self.state.argv))) - - if len(self.state.argv) != 2: - msg = 'Data item "{}" cannot be re-imported because of arguments number {} is incorrect. Argv: {}' - raise ReproError(msg.format(self._data_item.data.relative, len(self.state.argv), self.state.argv)) - - input = self.state.argv[0] - output = self.state.argv[1] - - cmd = CmdImportFile(self._settings) - cmd.set_git_action(True) - cmd.set_locker(False) - - Logger.info(u'Reproducing import command: {}'.format(output)) - if cmd.import_and_commit_if_needed(input, output, lock=True, - check_if_ready=False, - is_repro=True) != 0: - raise ReproError('Import command reproduction failed') - def reproduce_run(self): cmd = CmdRun(self._settings) cmd.set_git_action(True) @@ -195,14 +174,8 @@ class ReproChange(object): def reproduce_data_item(self): Logger.debug('Reproducing data item {}.'.format(self._data_item.data.dvc)) - self.remove_output_files() - - if self.state.is_import_file: - self.reproduce_import() - elif self.state.is_run: - self.reproduce_run() - # Ignore EMPTY_FILE command + self.reproduce_run() def reproduce(self): Logger.debug('Reproduce data item {}. recursive={}, force={}'.format( diff --git a/dvc/command/run.py b/dvc/command/run.py index 717509365..9cbeebb36 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -151,8 +151,7 @@ class CmdRun(CmdBase): Logger.debug('Create state file "{}"'.format(data_item.state.relative)) - state_file = StateFile(StateFile.COMMAND_RUN, - data_item, + state_file = StateFile(data_item, self.settings, input_set, output_files_dvc, diff --git a/dvc/state_file.py b/dvc/state_file.py index 028894185..1f2cdd891 100644 --- a/dvc/state_file.py +++ b/dvc/state_file.py @@ -21,11 +21,6 @@ class StateFile(object): DVC_PYTHON_FILE_NAME = 'dvc.py' DVC_COMMAND = 'dvc' - COMMAND_RUN = 'run' - COMMAND_IMPORT_FILE = 'import-file' - ACCEPTED_COMMANDS = {COMMAND_IMPORT_FILE, COMMAND_RUN} - - PARAM_COMMAND = 'Command' PARAM_TYPE = 'Type' PARAM_VERSION = 'Version' PARAM_ARGV = 'Argv' @@ -44,7 +39,6 @@ class StateFile(object): PARAM_MD5 = 'Md5' def __init__(self, - command, data_item, settings, input_items, @@ -67,10 +61,6 @@ class StateFile(object): self.code_dependencies = code_dependencies self.shell = shell - if command not in self.ACCEPTED_COMMANDS: - raise StateFileError('Args error: unknown command %s' % command) - self.command = command - self._argv = argv self.stdout = stdout @@ -135,14 +125,6 @@ class StateFile(object): except StateFileError: return None - @property - def is_import_file(self): - return self.command == self.COMMAND_IMPORT_FILE - - @property - def is_run(self): - return self.command == self.COMMAND_RUN - @property def argv(self): return self._argv @@ -180,8 +162,7 @@ class StateFile(object): @staticmethod def load_json(json, settings): - return StateFile(StateFile.decode_path(json.get(StateFile.PARAM_COMMAND)), - None, + return StateFile(None, settings, settings.path_factory.to_data_items(StateFile.decode_paths(json.get(StateFile.PARAM_INPUT_FILES, [])))[0], StateFile.decode_paths(json.get(StateFile.PARAM_OUTPUT_FILES, [])), @@ -213,7 +194,6 @@ class StateFile(object): self.update_target_metrics() res = { - self.PARAM_COMMAND: self.encode_path(self.command), self.PARAM_TYPE: self.MAGIC, self.PARAM_VERSION: self.VERSION, self.PARAM_ARGV: self.encode_paths(argv), diff --git a/functest/common.sh b/functest/common.sh index daead7842..38565fd9c 100644 --- a/functest/common.sh +++ b/functest/common.sh @@ -115,7 +115,8 @@ function dvc_create_repo() { dvc init mkdir data - dvc import $DATA_CACHE/* data + cp $DATA_CACHE/* data + dvc add data } function dvc_clean_cloud_aws() { diff --git a/functest/run.sh b/functest/run.sh index 0f235d26c..756d9be39 100755 --- a/functest/run.sh +++ b/functest/run.sh @@ -5,7 +5,7 @@ set -e source common.sh TESTS+=" test-init.sh" -TESTS+=" test-import.sh" +TESTS+=" test-add.sh" TESTS+=" test-sync.sh" TESTS+=" test-repro-code.sh" TESTS+=" test-merge.sh" diff --git a/functest/test-add.sh b/functest/test-add.sh new file mode 100755 index 000000000..3ab215cc2 --- /dev/null +++ b/functest/test-add.sh @@ -0,0 +1,13 @@ +#!/bin/bash + +set -e + +source common.sh + +dvc_create_repo + +dvc_info "Add file" +cp $DATA_CACHE/foo data/local +dvc add data/local +dvc_check_files "data/local .dvc/state/data/local.state .dvc/state/data/local.cache_state" +dvc_pass diff --git a/functest/test-import.sh b/functest/test-import.sh deleted file mode 100755 index bc277499b..000000000 --- a/functest/test-import.sh +++ /dev/null @@ -1,24 +0,0 @@ -#!/bin/bash - -set -e - -source common.sh - -dvc_create_repo - -dvc_info "Import from local file" -dvc import $DATA_CACHE/foo data/local -dvc_check_files "data/local" - -dvc_info "Import from http url" -dvc import $DATA_S3/foo data/http -dvc_check_files "data/http" - -if [[ "$TRAVIS_PULL_REQUEST" == "false" && "$TRAVIS_SECURE_ENV_VARS" == "true" ]]; then - dvc_info "Import from s3 url" - dvc config aws.region $REGION - dvc import s3://dataversioncontrol/functests/dummy_data/foo data/s3 - dvc_check_files "data/s3" -fi - -dvc_pass diff --git a/functest/test-merge.sh b/functest/test-merge.sh index 514e7a7fc..44771058c 100755 --- a/functest/test-merge.sh +++ b/functest/test-merge.sh @@ -16,7 +16,8 @@ cp code/code1.sh code/code4.sh git add code git commit -s -m"add code" -dvc import $DATA_CACHE/foo data/data +cp $DATA_CACHE/foo data/data +dvc add data/data ORIG_DATA=$(dvc_md5 data/data) @@ -31,7 +32,8 @@ git commit -am 'Set Target' # Create new branch to work on small data set git checkout -b bar dvc remove -c -l data/data -dvc import $DATA_CACHE/bar data/data +cp $DATA_CACHE/bar data/data +dvc add data/data dvc repro echo -e "\n\n\n\n" >> code/code1.sh diff --git a/functest/test-repro-code.sh b/functest/test-repro-code.sh index 618912972..4984eeaad 100755 --- a/functest/test-repro-code.sh +++ b/functest/test-repro-code.sh @@ -22,7 +22,8 @@ fi dvc_info 'Modify foo' dvc remove data/foo -dvc import $DATA_CACHE/bar data/foo +cp $DATA_CACHE/bar data/foo +dvc add data/foo dvc_info 'Set default target' dvc config global.target data/foo1