Skip to content
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

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Update HF upload script and model cards #239

merged 7 commits into from
Aug 7, 2024

Conversation

Sukh-P
Copy link
Member

@Sukh-P Sukh-P commented Jul 24, 2024

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 the wandb_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 to

if not is_ensemble:
        wandb_ids = wandb_ids[0]

so 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

@Sukh-P Sukh-P changed the title Update default for wandb_ids Update default for wandb_ids in HF upload script Jul 24, 2024
@Sukh-P Sukh-P requested a review from dfulu July 25, 2024 15:24
@Sukh-P Sukh-P marked this pull request as ready for review July 25, 2024 15:24
@Sukh-P Sukh-P requested a review from AUdaltsova July 30, 2024 13:46
@@ -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] = [],
Copy link
Contributor

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?

Copy link
Member Author

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

@dfulu
Copy link
Member

dfulu commented Aug 6, 2024

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?

@dfulu
Copy link
Member

dfulu commented Aug 6, 2024

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

@Sukh-P
Copy link
Member Author

Sukh-P commented Aug 6, 2024

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?

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

@Sukh-P
Copy link
Member Author

Sukh-P commented Aug 6, 2024

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 we agree, @AUdaltsova and I noticed this too and Alex has created an issue here for this #235

@AUdaltsova
Copy link
Contributor

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

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

@Sukh-P
Copy link
Member Author

Sukh-P commented Aug 7, 2024

FYI I am trying to tackle #235 in this PR too just to try and clean this up a bit more in one go

@Sukh-P Sukh-P changed the title Update default for wandb_ids in HF upload script Update HF upload script and model cards Aug 7, 2024
@Sukh-P Sukh-P requested a review from AUdaltsova August 7, 2024 11:41
@Sukh-P
Copy link
Member Author

Sukh-P commented Aug 7, 2024

Okay should be good to go now for a review with the additional changes, thanks

Copy link
Contributor

@AUdaltsova AUdaltsova left a 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!

Copy link
Member

@dfulu dfulu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good!

@Sukh-P Sukh-P merged commit d908019 into main Aug 7, 2024
3 checks passed
@Sukh-P Sukh-P deleted the update_to_hf_script branch November 5, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wandb_ids parsing doesn't work in scripts/checkpoint_to_huggingface.py
3 participants