-
Notifications
You must be signed in to change notification settings - Fork 3.5k
store: download model before load #16552
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
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the logic here makes sense
if not os.path.exists(_LIGHTNING_STORAGE_FILE): | ||
download_model(name=name, version=version, output_dir=_LIGHTNING_STORAGE_DIR, progress_bar=progress_bar) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks weird, we only want to download it if we haven't before right? Does download_model
handle that? Or can you only download one at a time?
Also, after you do this one that file will exist, but then if you change the model the file still exists no? So probably this only works the first time you do it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks weird, we only want to download it if we haven't before right? Does
download_model
handle that? Or can you only download one at a time?
good point, but I think this shall be handled on the download side...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the condition just before solving the issue?
if not os.path.exists(_LIGHTNING_STORAGE_FILE):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethanwharris 's point is that this has only a chance to run once, after which _LIGHTNING_STORAGE_FILE
will exist.
Note that _LIGHTNING_STORAGE_FILE
is defined as
_LIGHTNING_DIR = os.path.join(Path.home(), ".lightning")
__STORAGE_FILE_NAME = ".model_storage"
_LIGHTNING_STORAGE_FILE = os.path.join(_LIGHTNING_DIR, __STORAGE_FILE_NAME)
so it has nothing to do with the actual location where the specific model file has been saved. It only contains metadata, see:
The actual solution here is to peek into this file, extract the actual location and see if the specific model file exists. Then, if version is latest
, we need to resolve it and see if it increased with respect to what it's contained in the storage file.
for more information, see https://pre-commit.ci
not needed in light of major refactor in #18234 |
What does this PR do?
the actual store requires a user to call download manually before loading, so we will do it for them...
as an improvement, resolving #15811 (comment)
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃