-
Notifications
You must be signed in to change notification settings - Fork 32
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
Handle symlinks and excluded files in serialization #196
Comments
To clarify, I believe the code already supports symlinks as written (see #251), the TODO refers to preventing symlinks. model-transparency/model_signing/serialization/serialize_by_file_shard.py Lines 126 to 129 in 90e3ed3
This restriction was added in #24, and the only reason I can tell is from this block, which reads as a TODO to potentially allow symlinks:
It would be simple to add a toggle and check for them, but is it needed? Would a signer/verifier care about the presence of a symlink? |
There might be an issue when the model directory has a symlink inside it that points to a file outside. If we blindly read the symlink, we won't be able to verify the model after it gets uploaded, as that file won't be sent over. If we add a toggle to error when a symlink is found, we can detect this scenario earlier. I don't know if there are other cases where we'd care. If the model contains a symlink to a file within itself, nothing should change |
I assume this is heavily dependent on the upload process for a given model hub? My symlink knowledge is superficial at best, so I did a naive test with mkdir /tmp/foo
echo "HELLO" > /tmp/foo/bar.txt
mkdir /tmp/symlink-test
ln -s /tmp/foo/bar.txt /tmp/symlink-test/baz.txt
scp -r /tmp/symlink-test <host>:~ And then on the host, I see a normal file with the expected contents: $ cat symlink-test/baz.txt
HELLO
$ ls -laht symlink-test/baz.txt
-rw-r--r-- 1 <me> <group> 13 Jul 23 11:48 symlink-test/baz.txt
Sounds good. It's trivial to add the toggle, and it will be easy enough to remove if needed later. |
Yeah, |
Description
Following #190: during migration I did not yet add support for symlinks and for excluding files. This will be handled later when we find a need for it during end-to-end testing.
The text was updated successfully, but these errors were encountered: