Fix for 'upload_to_hf_hub()' path mismatch with 'save()' #3977
Conversation
Unit Test Results 6 files ±0 6 suites ±0 13m 40s ⏱️ -45s Results for commit aa1750e. ± Comparison against base commit b2cb8b0. This pull request skips 2 tests.♻️ This comment has been updated with latest results. |
for more information, see https://pre-commit.ci
…test_upload_to_hf_hub__validate_upload_parameters2()'
sanjaydasgupta
requested review from
w4nderlust,
tgaddair,
justinxzhao,
arnavgarg1,
geoffreyangus,
jeffkinnison,
Infernaught and
alexsherstinsky
as code owners
|
Hi @alexsherstinsky, please take a look. Thanks. |
ludwig/api.py
Outdated
| The description of the generated commit | ||
|
|
||
| # Returns | ||
|
|
||
| :return: (bool) True for success, False for failure. | ||
| """ | ||
| if os.path.exists(os.path.join(model_path, "model", "model_weights")) and os.path.exists( |
There was a problem hiding this comment.
@sanjaydasgupta The idea/direction seems great! I just have a couple of minor suggestions for you to consider (because if they work, they might improve the code quality slightly):
- Can we create the constants for the two directory names,
"model"and"model_weights"and replace codebase-wise? - Is it possible instead of passing the boolean
model_path_is_experiment_path, determine the "experiments" directory and standardize all methods on using it -- so the "experiment" directory would be passed to them. I think that even if a function is passed the "model" directory, and in those cases we have to make the assumption that "experiments" is "model/..", this would still be reasonable and acceptable. What do you think?
Thank you very much!
There was a problem hiding this comment.
@alexsherstinsky, responding to your (2) above - sounds like a great idea! Let me take a closer look at the code.
There was a problem hiding this comment.
Hi @alexsherstinsky, both your suggestions have been implemented.
There was already a label MODEL_WEIGHTS_FILE_NAME defined (and used in some places) for the literal string "model_weights". So the choice of a label for "model" fell naturally on MODEL_FILE_NAME -- not the best, but there it is for the time being.
There was a problem hiding this comment.
@sanjaydasgupta This is great! I left some comments/suggestions to see if we can make it even better -- thank you!
…y all support functions to assume "model_path" is experiment-path
for more information, see https://pre-commit.ci
…instead of literal strings "model" and "model_weights" respectively
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
@sanjaydasgupta I worry that it may not be safe to change a global constant as it may adversely impact backward compatibility (as in -- a user might rely on it). Is there any way for us in the present PR to constrain the changes in such a way that we do not modify the existing constants but reuse them and define a new one, if needed? Thank you!
I think there is a misunderstanding, let me clarify what I meant: The global constant MODEL_WEIGHTS_FILE_NAME (for the literal string “model_weights”) existed previously, and continues to be used to denote “model_weights”. I have not changed that. In fact I have replaced “model_weights” with the global constant in some more places in the code. I added the (previously unused) global constant MODEL_FILE_NAME for the literal string “model”. Most occurrences of “model” in the code have been replaced with the global constant. A few instances of “model” remain — their context was different (e.g. repository type etc.) My comment about their being poor choices was due to the use of “FILE” instead of “DIRECTORY” or “FOLDER”. Both constants refer to folders, not file-names. Please let me know if I misunderstood your comment. Thanks. |
There was a problem hiding this comment.
@sanjaydasgupta You are correct (I just got concerned about a 38-module change!). Please sync with the latest "master" branch, and I will approve. Thank you very much!
Hi @alexsherstinsky this PR is now ready for merge. |
There was a problem hiding this comment.
LGTM! Beautiful enhancement! Thank you very much, @sanjaydasgupta!
The code in this PR provides a solution for this issue: upload_to_hf_hub model path mismatch with model.save while also remaining backward compatible with the previous behavior of upload_to_hf_hub.
With this change,
upload_to_hf_hubcan accept any of the following kinds of path as itsmodel_pathargument:experimentfolder (existing implementation).modelfolder (added by this PR).The names
experimentandmodelrefer to places in the folder hierarchy created when a model is trained:results / experiment / model / model_weightsThe
modelfolder is additionally distinguished by containing themodel_hyperparameters.jsonfile. Themodel_weightsfolder contains one or more of these files:pytorch_model.bin,adapter_model.bin,adapter_model.safetensors.The changes to
upload_to_hf_hub()cause it to test themodel_pathargument to determine which kind of path was passed. If the path is amodelfolder, then the parent path (the correspondingexperimentpath) is obtained and used instead. So the logic ofupload_to_hf_huband its supporting functions do not have to change at all.It uses that knowledge for its own purposes and also passes it down to all supporting functions (as an additional argumentmodel_path_is_experiment_path: bool = True) for their use. The default value ofmodel_path_is_experiment_pathhas been chosen to ensure the backward compatible behavior.A unit test has been added (And the feature has been integration tested with custom code on a local build.test_upload_to_hf_hub__validate_upload_parameters2()) for the new use-case.