make 'cached_path' work offline #4253
Conversation
Co-authored-by: Michael Schmitz <michael@schmitztech.com>
| ) | ||
| latest_cached = _find_latest_cached(url, cache_dir) | ||
| if latest_cached: | ||
| return latest_cached |
There was a problem hiding this comment.
Is it worth logging something here? Users probably will not hit this often.
There was a problem hiding this comment.
In addition to the warning right above?
There was a problem hiding this comment.
No--my bad. Should we log the filepath of the file that will be used?
There was a problem hiding this comment.
Sure. Do you think INFO or WARNING level would be more appropriate?
There was a problem hiding this comment.
While I don't think this addresses the problem in #4234 (I believe the poster is running models on a system that has never had internet connectivity--I followed up with a solution), this seems like a great feature. It's always bothered me that we cannot use our cache while offline. While it's an edge case, it's super annoying when it happens (e.g. developing while traveling).
allennlp/common/file_utils.py
Outdated
| latest_cached = _find_latest_cached(url, cache_dir) | ||
| if latest_cached: | ||
| return latest_cached | ||
| logger.error( |
There was a problem hiding this comment.
This is a nit, but I prefer to have an else clause. I had trouble reading this in the CR because of this (an how Python handles blocks with spacing)--but admittedly it's just personal preference.
There was a problem hiding this comment.
Oh yea that's totally fine with me, I'm just used to certain linters raising a fit about "unnecessary else clause". I guess either flake8 doesn't care or we're explicitly ignoring that in our .flake8
This also does some additional clean up to
file_utils.py, in particular renaming the internal functions of the module with a leading underscore so that they won't be included in the API docs.