Closed Sukh-P closed 3 months ago
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
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
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 https://github.com/openclimatefix/PVNet/issues/235
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
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
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