fix PeftConfig loading from a remote repo. #649
Conversation
At least in huggingface-hub 0.10.1, the error for "not found" is: huggingface_hub.utils._errors.EntryNotFoundError: 404 Client Error
|
Thanks for the PR! I let @younesbelkada review it as he built that part of the repo - he's back from vacation next week. |
trl/models/modeling_base.py
Outdated
| @@ -263,7 +264,7 @@ class and the arguments that are specific to trl models. The kwargs | |||
| "pytorch_model.bin.index.json", | |||
| token=token, | |||
| ) | |||
| except ValueError: # not continue training, do not have v_head weight | |||
| except: # not continue training, do not have v_head weight | |||
There was a problem hiding this comment.
Can you elaborate why this change is needed?
There was a problem hiding this comment.
Hi @younesbelkada, thanks for getting back! This change has two reasons:
- At least in huggingface-hub 0.10.1, the error for "not found" is huggingface_hub.utils._errors.EntryNotFoundError, not a ValueError. This makes it fail to catch hf_hub_download() error when an entry is not found.
- This is also consistent with the code above (when trying to catch a similar scenario): https://github.com/huggingface/trl/pull/649/files#diff-be4f28845d341065eb254da5db9fa8fee2e6b5347071ef98448f27acd70fe8a7L166
There was a problem hiding this comment.
Thanks, that makes sense, would you be happy to import EntryNotFoundError from huggingface_hub similarly as here: https://github.com/huggingface/transformers/blob/main/src/transformers/utils/hub.py#L43 and replace the bare excepts with except EntryNotFoundError:? 🙏
There was a problem hiding this comment.
Hi @younesbelkada, I am thinking this would be a little tricky, as you might notice, there are many error codes, and the logics / handler is the same for any of them? So bare excepts will be reasonable in this case? Or we can just use the general Exception form?
There was a problem hiding this comment.
okay, I see only two error codes are sufficient for the purpose. I have made the proposed changes and applied it to other places in that file as well. Please review it in my most recent commit.
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Hi @w32zhong
Thanks a lot for iterating, sadly some CIs are failing, you can try to run:
pytest tests/test_peft_models.py::PeftModelTester::test_continue_training_peft_modelCan you please try to have a look 🙏 Let me know if you need any help !
|
@younesbelkada Sorry for getting back to you late. I've checked the exception that causes the failure of testcase. It is That being said, I have added HFValidationError into the catch cases. See if this will pass all testcases. |
|
Looks like this test failed: pytest tests/test_reward_trainer.py::RewardTrainerTester::test_reward_trainer_assert_value_errorBut a local run gives me error on the missing wandb api key. Any idea how to proceed? |
|
Hi @w32zhong |
|
@younesbelkada done. |
* fix PeftConfig loading from a remote repo. * failed to catch hf_hub_download() EntryNotFoundError. At least in huggingface-hub 0.10.1, the error for "not found" is: huggingface_hub.utils._errors.EntryNotFoundError: 404 Client Error * pass precommit checks. * replace some bare excepts with specific codes * catch LocalEntryNotFoundError additionally.
* fix PeftConfig loading from a remote repo. * failed to catch hf_hub_download() EntryNotFoundError. At least in huggingface-hub 0.10.1, the error for "not found" is: huggingface_hub.utils._errors.EntryNotFoundError: 404 Client Error * pass precommit checks. * replace some bare excepts with specific codes * catch LocalEntryNotFoundError additionally.
I am using peft 0.4.0 (currently the most updated one), it requires
PeftConfig.from_pretrained()to be passed a directory, instead of the config file path.