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

Write downloaded model files atomically #3247

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

naktinis
Copy link

Avoiding scenarios where one processes may be reading a file that is being written by another one by only moving the file to the final location after it has been fully written.

Avoiding scenarios where one processes may be reading a file that is being
written by another one by only moving the file to the final location after
it has been fully written.
@ekaf
Copy link
Contributor

ekaf commented Apr 19, 2024

@naktinis, does this PR solve a currently known problem? In particular, it would be great to know if it solves any of the many many open issues concerning NLTK download errors.

@naktinis
Copy link
Author

@ekaf this is an attempt to solve an issue we are having despite installing directly from develop branch (which already includes this unreleased fix: 69635b4).

The error we get almost every time is Error with downloaded zip file.

Our scenario is that we launch multiple processes simultaneously in an environment where there are no models downloaded yet.

So it appears the following sequence is quite likely in such circumstances (and my guess is that it's causing our error):

  1. process A checks if the model is downloaded – it is not, so clears the file and starts the download
  2. process B checks if the model is downloaded – it still is not, so clears the file and starts the download too
  3. process A finishes the download
  4. process A opens the model file – but process B is writing to it
  5. process A tries reading the content of the model file and fails because it is not fully downloaded

@naktinis
Copy link
Author

I couldn't see any directly related issues currenlty reported, but if you have any that seem relevant, I'm happy to take a look. I can also report this issue separately to have it associated with this PR if that's helpful.

@ekaf
Copy link
Contributor

ekaf commented Apr 19, 2024

@naktinis, your 5-point sequence above appears very plausible. However, if your assumption is correct, then we should expect this sequence to also have caused some of the many issues related to downloading and zip files, that have been reported over the past years.
Searching for "download" in issues yields this long list. Most are closed, but problems keep returning mysteriously. It seems likely that some of them resemble your scenario, and it would be really great if your solution could finally fix it.
Otherwise, it could indeed be quite helpful if you would consider opening a new issue to describe your specific problem, mentioning in particular which package you tried to install.

@naktinis
Copy link
Author

naktinis commented May 6, 2024

@ekaf I went through all open issues mentioning "download" and couldn't find anything obviously matching this, so I created an issue that includes a script to reproduce this: #3248.

@ekaf
Copy link
Contributor

ekaf commented May 7, 2024

Thanks @naktinis for the issue and the test script. Your scenario seems plausible, because the NLTK downloader dates back to a time where running multiple parallel processes was less common than now.

But given the large numbers of users who have had problems with downloading NLTK data packages, it seems surprising that none of them had similar errors. This would mean that you are downloading in a different way from everybody else, and I wonder what that could be?

Hopefully, some users with download problems will try your solution, and see if it works for them.

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.

None yet

2 participants