filesystem_spec icon indicating copy to clipboard operation
filesystem_spec copied to clipboard

Update dirfs.py: propagate transaction context to underlying filesystem

Open patrickwolf opened this issue 10 months ago • 6 comments

DirFileSystem now delegates its transaction and transaction_type to the wrapped filesystem, ensuring that with fs.transaction: on a dir:// instance correctly toggles the underlying file:// transaction. This restores atomic write semantics when using the directory wrapper.

Closes #1823 - the ticket also contains code to test the fix

patrickwolf avatar Apr 23 '25 03:04 patrickwolf

Can we please have a test to show that this works?

martindurant avatar Apr 24 '25 13:04 martindurant

Add tests for transaction context propagation in filesystem wrappers

This PR adds tests related to issue #1823 and its fix in PR #1824:

  1. test_dirfs_transaction_propagation: Verifies that DirFileSystem now correctly propagates transaction context to its wrapped filesystem, ensuring files are properly deferred until commit.

  2. test_cachingfs_transaction_missing_propagation: Documents that CachingFileSystem has a similar issue where transaction context is not propagated, causing immediate writes instead of deferred ones.

These tests both validate the current fix and identify areas for potential future improvements in the transaction handling system.

patrickwolf avatar Apr 24 '25 19:04 patrickwolf

I updated it in dirfs.py but really it also needs to be in cached.py and seeing that its in two classes there needs to be actually a base class for implementations that wrap existing file systems.

Currently they inherit from AbstractFileSystem or AsynFileSystem but some of them declare fs in the init method and others don't...

Any class that declares fs is a wrapper class and might need to inherit from the same class or interface so that wrappers can forward to implementations

patrickwolf avatar May 05 '25 06:05 patrickwolf

You are probably right, and we can later extract some of this common functionality for "wrapper" classes. Whether or not they delegate to the wrapped class will vary, though: write transactions for simplecache, for example, should write to local before committing, rather than depending on the remote storage's commit mechanism.

martindurant avatar May 07 '25 14:05 martindurant