-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update HF upload script and model cards #239
Conversation
scripts/checkpoint_to_huggingface.py
Outdated
@@ -21,7 +21,7 @@ | |||
def push_to_huggingface( | |||
checkpoint_dir_paths: list[str], | |||
val_best: bool = True, | |||
wandb_ids: Optional[list[str]] = None, | |||
wandb_ids: list[str] = [], |
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.
Out of curiosity: why not Optional anymore?
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.
That was a mistake, added it back in, thanks! Also updated the type hint slightly to the more up to date python way so we don't need to use Optional
This is a partial fix and it might be worth implementing a more complete one or making an issue to do so. The script adds the link to the wandb run in the huggingface model card. For this it uses a hard-coded wandb project and hf repo since it was written when we only used PVNet for UK regional. Having that automatically generated link between the model on HF and the actual training run is really good for traceability of models for us, and is also good for our open source-ness. I presume that you must be changing some of these hard coded paths locally in order to run this script anyway? |
I've just seen that the windnet and pvnet-india HF repos have used the model cards from PVNet UK which mention the model being used for UK regional GSP predictions. Also the links to the training runs lead to nowhere because of these hard coded values. So I think this should probably be a separate issue. Misusing the model cards like this is a bit messy of us |
Yep so we edit these locally when running the script as it wouldn't be put in the correct HF repo otherwise (for the HF one), I guess this could be done more cleanly and these be included as required parameters in the function itself to reduce the chance of some forgetting to change these |
Yep we agree, @AUdaltsova and I noticed this too and Alex has created an issue here for this #235 |
Can we maybe add choices/hints for it? Like "must be one of: path-to-pvnet-repo, path-to-windnet-repo" etc or even just in comments somewhere so you don't have to go look up the specific name on HF? Not that it's a lot of work but I'm a big fan of human laziness accommodation |
for more information, see https://pre-commit.ci
FYI I am trying to tackle #235 in this PR too just to try and clean this up a bit more in one go |
Okay should be good to go now for a review with the additional changes, thanks |
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.
Looks good to me!
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.
Looks really good!
Pull Request
Description
Have run into this a couple times now where I have ran the push checkpoint to HF script with just the
checkpoint_dir_path
parameter and I got an error about thewandb_ids
being None, I realised it didn't go into the logic to get the run ID starting at line 43 since that checks if it is an empty list and the default is None and then runs into an error if it's not an ensemble due toso I think changing it's default to an empty list might work a little more smoothly
Also removed some hardcoding of bits used in the model card and added separate model cards for different model types
Fixes #237 and #235