odb: ObjectDB cannot be equal

[pared]

Due to the following line:

dvc-objects/src/dvc_objects/db.py

Line 37 in b276ead

self.fs == other.fs

Two ObjectDBs will rather rarely be treated as equal. FileSystem does not have __eq__ method implemented. That will cause spawning multiple identical odb's in dvc, when, for example, fetching.

[pmrowla]

fsspec does have fs equality implemented using their internal _fs_token, @pared if you change that to be self.fs.fs == other.fs.fs does it resolve your issue?

(we need a better definition of __eq__ for dvc-objects filesystems, but I think that comparison was originally intended to be using the fsspec tokens)

[pared]

I can confirm that in the reproduction project provided in iterative/dvc#8178, comparing self.fs.fs with other.fs.fs yields true. So if we were to use this in ObjectDB.__eq__ we could probably limit number of ODB's generated by used_objs.

[Suor]

A related comment from a parallel issue: iterative/dvc#8178 (comment)

Building on that I would say that ObjectDB probably should not differ on some fs props, i.e. does it matter whether the fs is readonly or not? A readonly/readwrite otherwise the same pair of fses will contain the same objects.

[pmrowla]

Building on that I would say that ObjectDB probably should not differ on some fs props, i.e. does it matter whether the fs is readonly or not? A readonly/readwrite otherwise the same pair of fses will contain the same objects.

It shouldn't matter, but this was a workaround for fitting objects/ODBs into the pre-existing get_used_cache collection behavior.

During repo object collection (for push/pull/status) the existing workflow was to just collect all remotes/ODBs and all objects used in the repo in a given git commit, and we assumed that all objects could be both pushed and pulled to all remotes. This works for regular DVC remotes but not for repo imports (where we only want to pull from the source repo's remotes, but not push). So since everything uses the same collection method, we end up collecting imported objects/ODBs on push, but the actual push/write operation that happens later will skip pushing to any ODB marked as readonly.

The reason the readonly flag is used right now is because when we merge ODB's based on filesystems (which as noted by @pared is currently bugged), we still need a way to separate objects which are only meant to be read from a specific remote, and not pushed.

We can have a scenario where I have a single S3 bucket used as a shared remote across several DVC projects. If I dvc import from one of those other shared projects, I should still only be pulling objects from that project, and never trying to push those imported objects. But if we only use filesystem when merging the ODBs, I would end up with a single ODB for both my project's DVC remote, and the imported source repo's DVC remote (since they are using the same shared S3 bucket).

What we need is to revisit object collection but we haven't had time to get to that refactor yet. We should only collect objects/ODBs for the current operation (so dvc push only collects objects that can be pushed instead of all objects) and we should probably tie the read/write property to the objects instead of the ODB.

[Suor]

Readonly was only the example. The thing I was trying to say as long as they have the same location/url they should be the same for collection purposes. Otherwise we fail to dedup properly.