Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not overwrite prompt artifacts #53

Merged

Conversation

mynhardtburger
Copy link
Contributor

Issue: #51

Updates load_prompt_artifacts() to only copy new artifacts.
New artifacts are those which themselves (eg. foo.pt) or their swap/in-progress variant (eg. foo.pt.swp) already exist in the prompt_dir location.

Copying is done in two stages:

  1. Copy file with the .swp extension appended to indicate a copy is in progress
  2. Rename the file by removing the .swp extension after the copy is completed

Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Copy link
Contributor

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this one Mynhardt! My main question with how it's currently written is what advantage the .swp/rename offers over simply not copying if the named file itself already exists. Depending on the filesystem implementation, this could offer the advantage that the named file itself comes into existence atomically during the rename, but for other filesystems (most notably s3fs), don't guarantee an atomic rename, so that wouldn't always be the case.

@@ -57,7 +60,7 @@ class TGISConnection:
# Paths to client key/cert pair when TGIS requires mTLS
client_tls: Optional[TLSFilePair] = None
# TLS HN override
tls_hostname_override: str = None
tls_hostname_override: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

@@ -221,13 +224,29 @@ def load_prompt_artifacts(self, prompt_id: str, *artifact_paths: List[str]):
str,
artifact_paths=artifact_paths,
)
target_dir = os.path.join(self.prompt_dir, prompt_id)

target_dir = Path(self.prompt_dir) / prompt_id
Copy link
Contributor

Choose a reason for hiding this comment

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

One day I'll actually learn pathlib!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really like the path building using / :)

for artifact_path in artifact_paths:

# Don't copy files which are already in the target_dir
existing_artifact_names = {f.name for f in target_dir.iterdir()}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, do you happen to know if iterdir lazily evaluates the contents of the dir, or if it takes a proactive os.listdir and then iterates that (i.e. if the list changes during iteration, what happens)? Not particularly important in this instance, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iterdir() is a wrapper for os.listdir. So should be functionally the same:

    def iterdir(self):
        """Iterate over the files in this directory.  Does not yield any
        result for the special paths '.' and '..'.
        """
        for name in os.listdir(self):
            yield self._make_child_relpath(name)

@mynhardtburger mynhardtburger changed the title 51 do not overwrite prompt artifacts Do not overwrite prompt artifacts Feb 12, 2024
@mynhardtburger
Copy link
Contributor Author

mynhardtburger commented Feb 12, 2024

Thanks for digging into this one Mynhardt! My main question with how it's currently written is what advantage the .swp/rename offers over simply not copying if the named file itself already exists. Depending on the filesystem implementation, this could offer the advantage that the named file itself comes into existence atomically during the rename, but for other filesystems (most notably s3fs), don't guarantee an atomic rename, so that wouldn't always be the case.

Atomic rename guarantees that we don't see multiple states for the same file at the same time. But in our case it doesn't matter much:
Our primary goal is that foo.pt is complete and not in-progress. Whether a foo.pt.swp exists alongside it doesn't matter. That would be the non-atomicy showing itself. s3fs-fuse performs a server side copy for a rename. So, I assume, a pod crash won't impact that action and therefore we can rely on a foo.pt file to always reflect a complete file. Eventually the foo.pt.swp will disappear for all the pods.

On file systems which has atomic rename there is no downside.

If 2 pods happens to start a download for the same foo.pt at the same time, the os.replace() will silently override the older version irrespective of the OS.

@gabe-l-hart
Copy link
Contributor

s3fs-fuse performs a server side copy for a rename. So, I assume, a pod crash won't impact that action and therefore we can rely on a foo.pt file to always reflect a complete file.

Great! This makes a lot of sense

Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
@mynhardtburger mynhardtburger force-pushed the 51_Do-not-overwrite-prompt-artifacts branch from f00a8c0 to 9a77a8f Compare February 14, 2024 02:24
Signed-off-by: Mynhardt Burger <mynhardt@gmail.com>
Copy link
Collaborator

@evaline-ju evaline-ju left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution!

@evaline-ju evaline-ju merged commit 43f8174 into caikit:main Feb 14, 2024
6 checks passed
@evaline-ju evaline-ju linked an issue Feb 14, 2024 that may be closed by this pull request
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.

Do not overwrite prompt artifacts
3 participants