[RFC] Unify object store and file sources on a shared transport#23095
[RFC] Unify object store and file sources on a shared transport#23095nuwang wants to merge 3 commits into
Conversation
Introduce a FilesSource-backed object store so the object store and file source abstractions share one whole-file transport instead of duplicating a backend adapter per cloud. - Extend the FilesSource contract with exists/size/remove (posix native, fsspec base-class defaults), get_local_path (posix zero-copy resolve), and an optional usage_percent capability. - Add SourceObjectStore: owns a FilesSource and resolves get_filename via get_local_path, so a posix source is the disk role (zero copy, no cache) and a CachingFilesSource is the cloud role (cache path) -- one class, disk special-case gone. Handles addressing, store_by, alt_name, and always-local base_dir (job_work/temp) routing. - Extract CacheArea as the single cache implementation, shared by CachingConcreteObjectStore and CachingFilesSource. - Add DelegatingObjectStore + ObjectStoreTransport (Stage 1 seam) and a FilesSourceTransport adapter. - Register "type: source" in build_object_store_from_config; the source is built from a files_source plugin block with an optional cache wrapper. - Unit tests cover both roles via local, posix and fsspec (memory) sources, plus disk-store parity and build-from-config. Design rationale in doc/source/dev/objectstore_filesource_unification.md.
- Guard the FilesSource integration tests with pytest.importorskip so the galaxy-objectstore package can be tested in isolation without galaxy-files or fsspec (fixes "Test Galaxy packages"). - Import Iterable from collections.abc, not typing (ruff UP035). - Add the design doc to the dev docs toctree.
pytest collects _unification_utils.py directly, so its module-level galaxy.files imports must also be guarded with importorskip (not just the test modules that use them). Fixes the remaining "Test Galaxy packages" collection error.
| @@ -0,0 +1,207 @@ | |||
| """Stage 2 of the ObjectStore/FilesSource unification -- the collapse. | |||
|
|
|||
There was a problem hiding this comment.
Please de-claude these comments before pulling it out of draft:
https://github.com/jmchilton/claude-jmchilton-plugins/blob/main/plugins/jmchilton/commands/comment-archaeology.md
|
Is this work in a weird state for the SourceObjectStore was like a more integrated thing in Phase 2 that was support to replace the DelegatingObjectStore in Phase 1 and Claude just didn't clean up its own iterative/progressive work - or is there a reason to think through both and comment on both? As a development path the iteration makes sense and seems like a really smart way to build up the functionality but from the outside I'd rather just have to think through fewer intermediate states. My stress level on a SourceObjectStore being like the path forward for new object stores and an abstraction we build around is low. I think that is a fine idea - maybe even a great idea. I think there are some real design questions around should the thing that both FileSource and SourceObjectStores be plugged into be a new something that is maybe a light wrapper around fsspec stuff instead of SourceObjectStores talking directly to the FileSource and polluting the FileSource interface to the point where is is a full system interface. We keep going down that path and I keep resisting but maybe the battle is lost and FileSource is just our own parallel fsspec thing and the FileSource should just be the base thing. or My stress level raises a lot if we move on to actually replacing the object stores. Like can't we work on how to define stable interfaces between FileSources and ObjectStore configurations for the user facing and admin documentation components first. Can't we work on polishing one of these SourceObjectStores and getting it deployed in production and tested at scale before we starting merging everything. I worry a lot more about the interfaces and how these things are configured now that we're storing configurations in the database. Complete unification means every hack for an object store needs to be reflected as a seam through the FileSource and maybe not even just a particular FileSource but the whole interface to file sources and there are a lot of hacks in both of these layers. Are you doing this development for a particular implementation? If yes, maybe the concrete plea would be can we get that pieced together and working at scale and made into a User Defined Object Store and in a release and such before we start unifying everything. |
|
Thanks for going through this @jmchilton. Yes, some of the work is partial, and the design doc I was using as the basis is objectstore_filesource_unification.md. What I really wanted to do was to trigger a discussion on whether it's worth pursuing this path, because we had discussed it briefly earlier but never had a concrete prototype to anchor any discussion around, and it was also not obvious how involved it would be.
What got me started on it again are these changes: #23098 However, while doing so, it got me thinking again about the previous discussion we had on unifying the ObjectStore and FileSources implementations. Specifically, there's a noticeable amount of redundancy in handling the transport layer. I couldn't think of an obvious reason why most filesources couldn't be backing storage layers for an objectstore. And indeed, the UserObjectStoreTemplate and FileSourceTemplate also share the same code. So it seems like maybe that's because they are the same abstraction, the objectstore just does too many things? E.g. we could imagine configuring a bucket once as a filesourcetemplate, and then reusing that single definition for an object store too? Another major reason would be to continue along the path of allowing Galaxy to externalise ownership of data. I think that UserObjectStore's does this to some extent already, but perhaps gxfile uris would just complete the job, and would it be viable for Galaxy to move towards storing filesource uris as object ids? Admittedly, I'm suspicious that maybe I'm trying to conflate two abstractions that should ideally be separate, so I'm happy to have pushback on this. Just outlining my current thinking.
At this point, we already have some exceptions to this and since not all filesources are FsSpec driven (I'm probably at least partially to blame for this). I don't personally see much harm because I think filesystem interfaces are well-understood, and could be derived from FsSpec itself.
Agreed. And to be clear, I'm not even advocating strongly for a particular path. I just want to investigate viability, and whether it's worth pursuing. |
|
@nuwang I think it is worth pursuing - I just wanted to understand the whys. I also agree there is just a ton of redundancy across these abstractions. I don't think we will ever get it "right" but I'm happy to continue to discuss ways to improve it and I appreciate you pushing on this. |
RFC / prototype — opening as a draft to share the design and a working proof-of-concept for discussion.
What & why
Galaxy maintains two storage abstractions that have grown independently and now re-implement the same backend adapter per cloud: S3 exists as both
objectstore/s3_boto3.py(boto3) andfiles/sources/s3fs.py(fsspec), and the same duplication holds for Azure, GCS, and iRODS. Each cloud is maintained, tested, and credential-configured in two places.This introduces a shared whole-file transport that both abstractions sit on top of, without merging the two domain contracts — they encode genuinely different intents (the object store owns dataset bytes, is quota-bearing and cache-backed and must hand out a real local seekable path; a file source is a user-facing, browse/auth-heavy reach-out to foreign storage). The key result: a single object store class backed by a
FilesSource— a posix source gives theDiskObjectStorerole (zero-copyget_filename), a cache-wrapped remote source gives theS3ObjectStorerole — so the disk special-case disappears.Put differently, the idea it to make the relationship compositional. An ObjectStore's transport layer is delegated to a FileSource, so that each abstraction has a well-defined responsibility. Section 3.5 Reimplementation sketch:
DiskObjectStoreand 3.6 Reimplementation sketch:S3ObjectStoreinobjectstore_filesource_unification.mdare illustrative sketches.Full design rationale (including the "the seam already exists inside
CachingConcreteObjectStore" analysis and the staged rollout) is indoc/source/dev/objectstore_filesource_unification.md.What's implemented
exists/size/remove(posix native, fsspec base-class defaults so every fsspec source inherits them),get_local_path(posix zero-copy "source owns the path" resolve, kept distinct fromrealize_to), and an optionalusage_percent.SourceObjectStore— owns aFilesSourceand resolvesget_filenameviaget_local_path; handles addressing,store_by,alt_name(withsafe_relpath), and always-localbase_dir(job_work/temp) routing. Verified against theDiskObjectStorebehavioral contract.CachingFilesSource+ a singleCacheArea, now shared byCachingConcreteObjectStoreand the new source wrapper (one cache implementation instead of two).DelegatingObjectStore+ObjectStoreTransport/FilesSourceTransport— the narrow transport seam (Stage 1).type: sourcewired intobuild_object_store_from_config; the source is built from afiles_sourceplugin block with an optionalcachewrapper. This is intended as the safe adoption path: opt in per store via config, with no rewrite of the existing stores.Deferred (see doc §8)
The battle-tested
DiskObjectStore/S3ObjectStoreare not rewritten in place, and the duplicated cloud adapters are not yet deleted — that cutover should be gated by the full integration/e2e suite (andmotofor S3). Also deferred:dir_onlydataset create (composite / extra-files dirs), theobject_store_check_old_stylebackward-compat path, presignedget_object_urlfor the cloud role, relocating the cache primitives intogalaxy.files, and an s3fs-behind-motointegration test.How to test the changes?
New unit tests under
test/unit/objectstore/exercise both roles via local, posix, and fsspec (memory) sources, plusDiskObjectStoreparity (dataset lifecycle,store_by=uuid,alt_name, usage %, job-work dirs) and construction throughbuild_object_store_from_config.CI note: the substantive suites (Unit, Unit w/postgres, tool/workflow framework, toolshed, playwright), linting and docs all pass. The one red Test Galaxy packages job is a pre-existing, unrelated failure — the
web_appspackage cannot collectlib/galaxy/webapps/galaxy/api/mcp.pydue to a missing transitiveuncalled_for(pulled byfastmcp); this branch only unmasked it by fixing an earlier objectstore-package collection error.License