Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove: support file as a target #5288

Closed
DiPaolo opened this issue Jan 19, 2021 · 6 comments
Closed

remove: support file as a target #5288

DiPaolo opened this issue Jan 19, 2021 · 6 comments
Labels
A: data-management Related to dvc add/checkout/commit/move/remove enhancement Enhances DVC p3-nice-to-have It should be done this or next sprint

Comments

@DiPaolo
Copy link
Contributor

DiPaolo commented Jan 19, 2021

Description

As a further improvement of #4497, the support of using file as a target for dvc remove command should be added.

Here is what @skshetry says (#5236 (comment)):

we need to support file as a target in the remove, so I am not sure if this new error message makes sense.

We do need to clean up the message, especially the "failed to remove" one. So, either we need to add support for it, or just do the cleanup in this PR.

Regarding the support for files, the change should be pretty trivial, we just need collect_granular here, similar to what's in the repo.commit:

dvc/dvc/repo/commit.py (Line 44 in 228b3b1):
stages_info = self.stage.collect_granular( )

@efiop made another point (#4497 (comment)) based on a user's feedback:

Another user ran into it https://discord.com/channels/485586884165107732/485596304961962003/798834932217937970

/cc @efiop @skshetry

@pared pared added enhancement Enhances DVC good first issue p3-nice-to-have It should be done this or next sprint labels Mar 31, 2021
daavoo added a commit to daavoo/dvc that referenced this issue Apr 1, 2021
@daavoo daavoo added the A: data-management Related to dvc add/checkout/commit/move/remove label Feb 22, 2022
@alexmojaki
Copy link
Contributor

I was able to make this work with the following change to dvc/utils/__init__.py:

@@ -382,8 +382,12 @@ def parse_target(
                 )
             )
         if not name:
-            ret = (target, None)
-            return ret if is_valid_filename(target) else ret[::-1]
+            if is_valid_filename(target):
+                return target, None
+            elif os.path.isfile(target) and os.path.isfile(target + DVC_FILE_SUFFIX):
+                return target + DVC_FILE_SUFFIX, None
+            else:
+                return None, target

However it breaks the tests test_collect_granular_same_output_name_stage_name and test_gitignored_file_try_collect_granular_for_dvc_yaml_files. I'm guessing such a fundamental change has undesirable implications for the rest of the project. So should changes be isolated to dvc/repo/remove.py? Or is there somewhere else where it would be good to interpret a non-dvc file as a stage?

BTW, should dvc remove foo.bar just be equivalent to dvc remove foo.bar.dvc (when such a file exists) or should it do more, such as actually delete the file foo.bar?

@dberenbaum
Copy link
Collaborator

BTW, should dvc remove foo.bar just be equivalent to dvc remove foo.bar.dvc (when such a file exists) or should it do more, such as actually delete the file foo.bar?

It can just be equivalent to dvc remove foo.bar.dvc. There is already the --outs option to also delete foo.bar.

@daavoo
Copy link
Contributor

daavoo commented Jul 5, 2022

Note that the last time I tackle this the conclusion was that dvc remove semantics were (more) unclear with this additional feature and decided to drop the P.R. (see iterative/dvc.org#2357 (comment))

@dberenbaum
Copy link
Collaborator

Thanks for remembering that @daavoo. I would echo that it may not be worth the effort at the moment and may require more holistic changes to the UI.

@alexmojaki
Copy link
Contributor

Here's a small change that only improves the error message: main...alexmojaki:dvc:remove-file-message

Now when you run dvc remove thing, instead of:

ERROR: 'dvc.yaml' does not exist

it says

ERROR: 'thing' is not a .dvc file Do you mean 'thing.dvc'?: 'dvc.yaml' does not exist

and similarly instead of

ERROR: Stage 'thing' not found inside 'dvc.yaml' file

it says

ERROR: 'thing' is not a .dvc file Do you mean 'thing.dvc'?: Stage 'thing' not found inside 'dvc.yaml' file

@dberenbaum
Copy link
Collaborator

Great idea @alexmojaki! Let's go with that, thank you 🙏 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: data-management Related to dvc add/checkout/commit/move/remove enhancement Enhances DVC p3-nice-to-have It should be done this or next sprint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants