Check if file exists before sorting them into a list #784
Conversation
…ramalama ls Signed-off-by: Kush Gupta <kushalgupta@gmail.com>
kush-gupt
requested review from
rhatdan,
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver,
slp and
engelmi
as code owners
Reviewer's Guide by SourceryThis pull request introduces a safety check before adding file paths to the list used for sorting by modification date, preventing errors caused by dangling symlinks or non-existent files. Flow Diagram for File Existence Check in List Files Operationflowchart TD
A[Start: Iterate over file paths] --> B{Does file exist?}
B -- Yes --> C[Append file path to list]
B -- No --> D[Output error: path does not exist]
C --> E[Continue processing]
D --> E
E --> F[Sort list by modification date]
F --> G[Return sorted list]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kush-gupt - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider logging when a file is skipped due to not existing, to aid in debugging.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Fixes the issue reproduced here: #782 (comment) |
|
Btw, if we get to the symlink -> hardlink change, that's gonna require a significant rewrite. The logic relies on following symlinks to remove all occurrences of a model. I wonder should we auto-cleanup broken symlinks somewhere to stop them building up. The whole :latest tag thing being an alias to another tag needs to be taken into account. If we delete one tag, we shouldn't necessarily delete the backing file. We can change to hardlinks, but hardlinks don't follow each other, we have to search the filesystem for the other inodes. |
|
In the context of where the other inodes are located, it should be just the ollama and hf caches right? So the user would have to ollama/hf rm them separately to completely wipe the model from their filesystem but that's also what we'd expect right? Scenario I see is: User pulls some model with ramalama, it's nice to load from cache but it's still on the source cache/tooling to manage the original model? |
|
I was thinking of creating a hard link to a file in the same destination to where ramalama pull ollaman:foobar puts it now. So you would still have a symlink, but instead of a symlink to the file under OLLAMA it would be a symlink to a hardlink file under $HOME/.local/share/ramalama |
|
With this change does |
|
ramalama rm works to remove the broken symlink: |
I'm not sure there is a case where broken symlink happens. I suspect @kush-gupt just created a dangling symlink. I just want to take a moment to describe why hardlinks have different disadvantages. The problem with hardlinks is the Ollama structure: 8b and latest are pointing to the same file, both just symlinks for de-duplication purposes. If we make things hardlinks, we have to do a lot of searching for the other linked files, it will be expensive. We don't really gain the automatic reference counting cleanup capabilities of hardlinks because we don't refer to models by their checksum (and we have to find that checksum somehow, with symlink it's easy because you are pointed to that SHA, when it's a hardlink we aren't pointed to any sha) |
|
IIRC the "ramalama rm" does a decent job of trying to delete orphaned things, which is why here we see two sha256's deleted in @kush-gupt 's output |
|
Indeed, I had that broken symlink created after I did |
|
Ok I think I am ok with just symlink as long as RamaLama handles it. I think we also create a symlink to a model stored as a file. file:///tmp/model.gguf for example. |
|
I would like to see a test for this though. Create a model with file://$TEMPDIR/model.gguf Show it in list. Remove $TEMPDIR/model.gguf |
Signed-off-by: Kush Gupta <kushalgupta@gmail.com>
|
Added a call to remove if a broken symlink is detected when listing models: |
|
LGTM |
A dangling symlink can cause
ramalama lsto fail with:So checking to see if the file exists before appending it into a list to be sorted fixes this:
Root cause could be solved by creating a hard link instead of symbolic from to ollama model blob to the ramalama blob store
Summary by Sourcery
Bug Fixes:
ramalama lsto fail.