fix file upload on windows #45
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NateVolt
changed the title
|
Thanks a lot for your contribution. Really smart and simple ! It works on macos :) |
Shaunwei
pushed a commit
to Shaunwei/quiver
that referenced
this pull request
May 20, 2023
* defer marking temp file for deletion closes QuivrHQ#38 * manually delete file
StanGirard
pushed a commit
that referenced
this pull request
Sep 12, 2023
* defer marking temp file for deletion closes #38 * manually delete file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #38
I did some digging and this seems to be a recurring theme with
tempfile.NamedTemporaryFile()not being completely cross-platform. According to the python docs you can't open a tempfile more than once on windows.I aimed to make the minimum viable change that would fix the issue. All I have done here is to disable automatic cleanup and then clean it up manually. Some commenters on S.O. suggested rolling your own to avoid the cross-platform issues. According to official python docs
os.remove()is fully cross-platform. However, I see that this is built on top of langchain, which seems to make heavy use oftempfile.NamedTemporaryFile()so it may not be feasible to migrate to a different file handling approach. I haven't scanned the rest of the Quivr codebase yet, but if this is the only place where file I/O occurs then this patch might be enough to close the books on this.I tested on my machine by uploading a csv, txt, and pdf. I watched as each file was placed in the temp directory and then saw each get cleaned up.
Sidenote: I am not a python expert, so maybe this is normal, but I would expect
tmp_fileto be out of scope upon exiting thewithblock. However, I had no issues using it after exiting that scope. Anyone see anything concerning here?