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

Consider using cached-path crate in resources module #72

Closed
epwalsh opened this issue Sep 4, 2020 · 6 comments · Fixed by #74
Closed

Consider using cached-path crate in resources module #72

epwalsh opened this issue Sep 4, 2020 · 6 comments · Fixed by #74

Comments

@epwalsh
Copy link
Contributor

epwalsh commented Sep 4, 2020

Hey @guillaume-be, I wrote a Rust version of the functionality you find in file_utils.py from transformers (and allennlp) called cached-path. This could make the resources module more robust. cached-path knows when to update a resource based on the Etag, and ensures the cache is never corrupted if a download fails for any reason. It also avoids race conditions by using file locks.

Let me know what you think.

@guillaume-be
Copy link
Owner

Hi @epwalsh nice crate and seems to align well with the current resources downloader. The check for Etag and file locks is a nice addition.

I am trying to be mindful of the dependencies pulled in the project. I see you are relying on the failure crate which has been deprecated for a few months now. Is there any way you could rely instead on the thiserror crate for your API error handling, and anyhow for dev/examples?

@epwalsh
Copy link
Contributor Author

epwalsh commented Sep 8, 2020

Hey @guillaume-be, yeup, that sounds good. Didn't realize failure has been deprecated.

@epwalsh
Copy link
Contributor Author

epwalsh commented Sep 9, 2020

Just made a PR to switch over: epwalsh/rust-cached-path#18

@guillaume-be
Copy link
Owner

@epwalsh Thank you - this looks good. I also noticed that the crate currently does not have further dependencies - which is fine on my side. Because of this I believe a good unit test and integration test coverage would be extremely helpful. Could you please add test for key functionalities of the crate (e.g. file lock, cache corruption, Etag check) in either unit or integration tests and add them to the CI?

@epwalsh
Copy link
Contributor Author

epwalsh commented Sep 10, 2020

Hey @guillaume-be, are you asking me to add tests to the cached-path crate? We have already some tests here: https://github.com/epwalsh/rust-cached-path/blob/master/src/cache.rs#L433

@guillaume-be
Copy link
Owner

Sorry I missed those!

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 a pull request may close this issue.

2 participants