Skip to content

Fix MemoryFileSystem.makedirs accepting existing files - #2267

Merged
martindurant merged 5 commits into
fsspec:masterfrom
Ganesh1729-ui:fix/2266-memory-makedirs-file
Oct 14, 2026
Merged

martindurant merged 5 commits into
fsspec:masterfrom
Ganesh1729-ui:fix/2266-memory-makedirs-file

Conversation

@Ganesh1729-ui

@Ganesh1729-ui Ganesh1729-ui commented Oct 11, 2026 •

Copy link
Copy Markdown

Fixes #2266.

Summary

MemoryFileSystem.makedirs(..., exist_ok=True) returned success when the target was an existing file. The target still was not a directory, contrary to the base-method contract and local filesystem behavior. Re-raise FileExistsError when the normalized target is in the memory file store; existing directories remain accepted. This preserves the cache suite's no-metadata-lookup behavior.

The 12 regression cases cover relative, absolute and memory:// paths, both exist_ok values, and the makedirs/mkdirs entry points. Each checks that existing file contents remain intact. A Dev changelog entry describes the fix.

Verification

  • Six new cases failed on unchanged original source; all 12 pass with the fix.
  • Merged current master at 46c05cb without a force push. The PR diff remains three files, 16 additions and one deletion.
  • Full available suite on the merged head: 2,200 passed, 303 skipped, 13 xfailed. Optional service and platform cases account for the skips.
  • Following maintainer review, the collision check now uses self.isfile(path), whose implementation normalizes the path and directly checks the in-memory store. The focused memory/cache modules pass 204 tests, 36 skips; the full suite remains 2,200 passed, 303 skipped, 13 xfailed on the reviewed head. Configured hooks pass. Earlier strict Sphinx, source/wheel build and isolated-wheel collision checks passed before the base sync.

AI assistance was used to investigate and prepare the patch. I reviewed the merge resolution and reran the suite and hooks.

@Ganesh1729-ui

Copy link
Copy Markdown
Author

Validation summary for e9241e56: the 12 file-collision cases cover relative, absolute and memory:// paths, both exist_ok values and both directory-creation entry points. Six cases fail on unchanged source; the fixed available suite passes 2,119 tests versus 2,107 on the baseline, with the same 296 skips and 13 xfails.

The normalized store-membership check also preserves the cache suite's no-metadata-lookup assertion. Configured hooks, strict Sphinx, package builds and isolated-wheel checks pass. Optional service/platform skips remain documented. Upstream workflow approval and maintainer review are still pending.

@Ganesh1729-ui

Copy link
Copy Markdown
Author

I merged current master at 8b8bd82 into this branch and resolved the Dev changelog conflict, preserving the new upstream entries and this fix's #2266 entry. The published source change is still the single makedirs condition and its 12 regression cases; the branch is now at 2cade207.

On the merged head, the memory/cache modules pass 187 tests with 36 skips; the full available suite passes 2,141 tests, 296 skips and 13 xfails. All configured hooks pass on the three changed files. No force push or unrelated source adjustment was made.

Comment thread fsspec/implementations/memory.py Outdated
self.mkdir(path, create_parents=True)
except FileExistsError:
if not exist_ok:
if not exist_ok or self._strip_protocol(path) in self.store:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as self.isfile ? Might be clearer.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; MemoryFileSystem.isfile() already normalizes the path and checks self.store directly, so it expresses the intent without adding a metadata lookup. I changed the condition to self.isfile(path) in e17798b. The focused memory/cache modules pass (204 passed, 36 skipped), the full available suite passes (2,194 passed, 298 skipped, 13 xfailed), and the configured hooks pass. Thanks for the clearer spelling.

@Ganesh1729-ui

Copy link
Copy Markdown
Author

The base advanced again after my previous update, so I merged master at 8cf902e into this branch. The only conflict was the Dev changelog block; I kept all new upstream entries and the #2266 entry. The PR still changes only the makedirs condition, its 12 regression cases, and the changelog (three files, 16 additions/one deletion).

On the new db01e63 head, the full available suite passes 2,194 tests with 298 skips and 13 xfails; all configured hooks pass on the three changed files. The branch was pushed normally, with no force push. The PR description now records this head's validation.

@Ganesh1729-ui

Copy link
Copy Markdown
Author

Merged current master (46c05cb) into this branch and resolved the Dev changelog conflict, preserving both new upstream entries and the memory makedirs entry. Current head is e123947; the PR diff remains three files, 16 additions and one deletion. The reviewed self.isfile(path) check remains intact.

Validation on the merged source: memory/cache modules 204 passed / 36 skipped; full available suite 2,200 passed / 303 skipped / 13 xfailed; all configured hooks pass. The PR description now reflects this head and these results.

@martindurant
martindurant merged commit cd9713e into fsspec:master Oct 14, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MemoryFileSystem.makedirs(exist_ok=True) accepts an existing file

2 participants