Skip to content

Conversation

@AsherJingkongChen
Copy link

Fixes #1827

Creates temp file in the same directory as the target to prevent EXDEV errors
when temp and target are on different filesystems (e.g., checkpoints to NFS).

Changes

  • mkstemp uses dir=os.path.dirname(self.path) with filename prefix
  • Removed PermissionError workaround since EXDEV won't occur
  • chmod catches PermissionError for filesystems without permission support
  • Test verifies temp file location directly

Context

Similar approach was in #1829 initially. As noted there, this only affects local.py.

Copy link
Member

@martindurant martindurant left a comment

Choose a reason for hiding this comment

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

Would you say there are times to prefer /tmp over the target directory, for instance for NFS mounts that are slow and don't like many writes?

else:
# TODO: check if path is writable?
i, name = tempfile.mkstemp()
i, name = tempfile.mkstemp(
Copy link
Member

Choose a reason for hiding this comment

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

If we are not using a system temporary location, we could just use normal open() and avoid the chmod call further down. The only thing mkstemp is doing for us here is generating the filename with some uuid.

Copy link
Author

Choose a reason for hiding this comment

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

keeping mkstemp is fine since it still works and avoids breaking umask cache tests

@AsherJingkongChen
Copy link
Author

Would you say there are times to prefer /tmp over the target directory, for instance for NFS mounts that are slow and don't like many writes?

@martindurant Both approaches write filesize to NFS. This PR just skips the extra copy step. Can't think of a case where /tmp would be better.

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.

Invalid cross-device link

2 participants