version: improve link type detection fall back #4479
Conversation
dvc/command/version.py
Outdated
| if create_tmp_cache_dir: | ||
| shutil.rmtree(repo.cache.local.cache_dir, ignore_errors=True) |
There was a problem hiding this comment.
A bit worried about this racing in shared cache scenarios, where cache dir might be accidentally deleted. Also, we might simply not have any rights to create that cache_dir, so btter to wrap makedirs above in a try&except logic. Also, we will be leaving traces in a form of parent directories that didn't exist before. But if we are not able to create even a temporary dir, then we will still need to show some message about it, which is actually the same thing as was suggested in the original ticket by @shcheklein as a first option. That makes me wonder if we really shouldn't overcomplicate this and just stick to the first option and just not even try to create a temporary directory. Sorry for not suggesting it earlier 🙁
There was a problem hiding this comment.
Thanks for detailed explanation about shared cache scenario.
Just to be sure I understood your message correctly. The final solution is to remove warning completely. No new code required. Is it correct?
There was a problem hiding this comment.
@IvanRubanov Yes, I think removing it and, for example, adding something like
Cache types: https://error.dvc.org/unable-to-detect-cache-type
to the dvc version output would be the best overall solution here in terms of complexity and effectiveness. Thank you so much for looking into this!
There was a problem hiding this comment.
Hi @IvanRubanov !
Thank you so much for contributing! 🙏 Please see a comment above.
|
@efiop could you check this PR once again? |
dvc/command/version.py
Outdated
| info.append( | ||
| "Cache types: " | ||
| + "https://error.dvc.org/unable-to-detect-cache-type" |
There was a problem hiding this comment.
| info.append( | |
| "Cache types: " | |
| + "https://error.dvc.org/unable-to-detect-cache-type" | |
| info.append( | |
| "Cache types: " + error_link("unable-to-detect-cache-type") |
error_link helper is defined in dvc.utils. Could you also add such a section to https://dvc.org/doc/user-guide/troubleshooting, please? There is an edit button at the bottom right corner.
There was a problem hiding this comment.
Done. Updated PR + created new PR for web site documentation.
If failed to determine cache type print link to related web page. Fixes #2788
dvc/command/version.py
Outdated
| logger.warning( | ||
| "Unable to detect supported link types, as cache " | ||
| "directory '{}' doesn't exist. It is usually auto-created " | ||
| "by commands such as `dvc add/fetch/pull/run/import`, " | ||
| "but you could create it manually to enable this " | ||
| "check.".format(relpath(repo.cache.local.cache_dir)) | ||
| info.append( | ||
| "Cache types: " + error_link("unable-to-detect-cache-type") |
There was a problem hiding this comment.
Hi! Should some info on what the problem is be presented to the user at least? Other than just Cache types: http://error... Maybe I'm not seeing the full output just from this code.
There was a problem hiding this comment.
Also could we shorten the link path a bit? Maybe "no-dvc-cache"
There was a problem hiding this comment.
(Please see also iterative/dvc.org#1751 (review).)
There was a problem hiding this comment.
@jorgeorpinel Adding more info will defeat the purpose of this PR, will make the desc too long.
|
Thank you @IvanRubanov ! 🙏 |
Create temporary file in cache dir. And use it to detect the supported cache types. Clear after cache type detection.
Fixes #2788