config: don't set default cache dir #9037
Conversation
dvc/cachemgr.py
Outdated
| default = None | ||
| if repo and repo.dvc_dir and not isinstance(repo.fs, GitFileSystem): | ||
| default = repo.fs.path.join(repo.dvc_dir, self.CACHE_DIR) | ||
| elif repo: | ||
| dvc_dir = os.path.join( | ||
| repo.scm.root_dir, | ||
| *repo.fs.path.relparts(repo.root_dir, "/"), | ||
| repo.DVC_DIR, | ||
| ) | ||
| if os.path.exists(dvc_dir): | ||
| default = os.path.join(dvc_dir, self.CACHE_DIR) | ||
|
|
There was a problem hiding this comment.
Not the best look and there are lots of isinstance(repo.fs, GitFileSystem) across the codebase elsewhere, but the proper solution will have to wait. Will probably get back to this when messing with repo.tmp_dir for persistent index.
There was a problem hiding this comment.
I find the whole Repo() constructor taking a "git" filesystem to be a wrong approach, needlessly complicating things. It should require a physical filesystem during instantiation.
There was a problem hiding this comment.
@skshetry That would not cover bare repos. We need to be able to operate on virtual filesystems.
There was a problem hiding this comment.
We just need to clearly handle "workspace-fs" (e.g. where the scm is, odb is, etc) from "vfs" (where dvcfiles are, some configs are, etc). It will get better from here.
There was a problem hiding this comment.
Bare repos aren’t that important, we could easily drop support (or, clone if needed).
I am not saying dvc not to support virtual filesystems, but loading config always requires a working filesystem. So, Repo.__init__ could always require a physical filesystem.
The other cases could be handled through different classmethods: Repo.from_git() etc.
There was a problem hiding this comment.
Ah, re-read your message. Yeah, I agree, the alternative is to just pass a rev and scm or something. Also looked at it a bit. Might get back to it later (external_repo is also annoying).
efiop
force-pushed
the
config-cache
branch
2 times, most recently
from
e5d721e to
340e352
Compare
Codecov ReportBase: 93.04% // Head: 92.98% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #9037 +/- ##
==========================================
- Coverage 93.04% 92.98% -0.06%
==========================================
Files 456 456
Lines 36758 36773 +15
Branches 5322 5208 -114
==========================================
- Hits 34201 34193 -8
- Misses 2030 2049 +19
- Partials 527 531 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
efiop
force-pushed
the
config-cache
branch
2 times, most recently
from
db6215c to
cc81681
Compare
Setting default values is very untypical for all other applications and the info config has is not enough to make a decision on the default cache dir, especially when dealing with gitfs. Fixes iterative#8705
Setting default values is very untypical for all other applications and the info config has is not enough to make a decision on the default cache dir, especially when dealing with gitfs.
Fixes #8705