output: remove: catch FileNotFoundError #9574
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #9574 +/- ##
=======================================
Coverage 90.53% 90.53%
=======================================
Files 471 471
Lines 36068 36074 +6
Branches 5185 5185
=======================================
+ Hits 32654 32660 +6
Misses 2823 2823
Partials 591 591
☔ View full report in Codecov by Sentry. |
dberenbaum
force-pushed
the
output-recursive-false
branch
from
e78c274 to
542d9df
Compare
|
It looks like the remaining failing tests are unrelated. |
dvc/output.py
Outdated
|
|
||
| def remove(self, ignore_remove=False): | ||
| self.fs.remove(self.fs_path, recursive=True) | ||
| self.fs.remove(self.fs_path, recursive=False) |
There was a problem hiding this comment.
We've discussed this before, but it loks wrong. E.g. imagine a directory with files as an output, this will error-out (at least unless I'm missing something). Could you elaborate on the problem that you are trying to solve, please?
There was a problem hiding this comment.
That's what the test tries to cover unless I'm misunderstanding you, but we probably do need to test it across other filesystems.
The problem is in #8757 - the recursive call errors out for a nonexistent path in s3fs.
As mentioned above, I don't mind closing if we come up with a different approach to solve it in s3fs or fsspec, but wanted to open it for discussion as a potential quick fix.
There was a problem hiding this comment.
I think you are spot on in your explanation in the issue. localfs is ignoring that flag (looks like our oversight) and since all other filesystems would also raise FileNotFoundError, we should just catch and ignore FileNotFoundError here as well.
| self.fs.remove(self.fs_path, recursive=False) | |
| try: | |
| self.fs.remove(self.fs_path, recursive=True) | |
| except FileNotFoundError: | |
| pass |
The test you've added could be removed or kept around. Ideally, we would test external outputs in dvc.testing, but that will probably just have to wait for better times.
There was a problem hiding this comment.
The test will become relevant once we make our localfs comply with the flag, but IIRC there are a few more places that need to be fixed along the way, so that could wait.
efiop
changed the title
|
Thank you! |
2 participants
Fixes #8757. Feel free to close and take a different approach. Trying to find a quick fix that will make non-cached external outputs work as expected.