meta: add version_id #153
meta: add version_id #153
Conversation
Codecov Report
@@ Coverage Diff @@
## main #153 +/- ##
==========================================
- Coverage 55.43% 55.31% -0.12%
==========================================
Files 41 41
Lines 2345 2350 +5
Branches 423 424 +1
==========================================
Hits 1300 1300
- Misses 965 976 +11
+ Partials 80 74 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
pmrowla
force-pushed
the
meta-version-id
branch
from
6113626 to
5441a64
Compare
pmrowla
force-pushed
the
meta-version-id
branch
from
5441a64 to
8549898
Compare
pmrowla
force-pushed
the
meta-version-id
branch
from
8549898 to
8a9e757
Compare
src/dvc_data/hashfile/state.py
Outdated
| if fs.version_aware: | ||
| _, version_id = fs.path.split_version(path) | ||
| else: | ||
| version_id = None |
There was a problem hiding this comment.
State does not support non-local filesystems, so I don't think we need to add support for this. It's a dead code. :)
There was a problem hiding this comment.
My understanding is that this is needed for some of the things @efiop is working on
There was a problem hiding this comment.
Yeah, let's keep this (using this in POCs, but could probably turn this on later too).
There was a problem hiding this comment.
I'd prefer not adding this dead code. In POC, you can cherry-pick easily, no need to add to main.
We don't even support non-local filesystems, version-aware fs is too far.
There was a problem hiding this comment.
The design of the State might even change, or there may be no state in the future.
There was a problem hiding this comment.
@skshetry It is very close actually 🙂
There was a problem hiding this comment.
Though version_ids are problematic for state in https://github.com/iterative/dvc-data/pull/153/files#r958293617 so we might indeed want to not touch it in this PR.
pmrowla
force-pushed
the
meta-version-id
branch
from
8a9e757 to
75ea731
Compare
src/dvc_data/hashfile/state.py
Outdated
| if fs.version_aware: | ||
| _, version_id = fs.path.split_version(path) |
There was a problem hiding this comment.
The version id should probably be extracted from info call (same as size and etag in the future). But I guess this is a bit debatable and when version_id is specified in the path - maybe we need to treat it as a path...
There was a problem hiding this comment.
Also version_aware is only present for some filesystems, you'll get an attribute error.
There was a problem hiding this comment.
Looks like we might want to treat path with version_id as a complete path here, because you might have a situation where you have s3://bucket/path and s3://bucket/path?version_id=12434 at the same time and state should actually care about both. Hm...
There was a problem hiding this comment.
Also
version_awareis only present for some filesystems, you'll get an attribute error.
This is addressed in the open dvc-objects PR https://github.com/iterative/dvc-objects/pull/133/files#
There was a problem hiding this comment.
and yes we would need to use the versioned path with state, or modify lookups to work like
def get(self, path, fs, version_id=None):
(or potentially meta=None)
|
It sounds like we should just drop the state changes and follow up on that later, and then merge this + the dvc-objects PR for now |
pmrowla
force-pushed
the
meta-version-id
branch
from
75ea731 to
e65ffeb
Compare
|
This depends on the dvc-objects PR and can be merged after that is released iterative/dvc-objects#133 |
No description provided.