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

Refactor serialization via Huggingface Hub #75

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

agijsberts
Copy link
Contributor

Pull Request

Description

This PR follows the discussion in #68 and makes DGMR compatible with recent updates in huggingface_hub.

Specifically, it removes the explicit config argument from constructors. This simplifies the constructors, but may cause some backwards compatibility problems if people rely on passing this parameter. However, it should be easy to fix this by replacing DGMR(config=myconfig) with DGMR(**myconfig).

Secondly, this PR also contains a separate commit that replaces the custom NowcastingModelHubMixin with huggingface_hub.PyTorchModelHubMixin. This does not have any real advantages, aside from trimming redundant code. It should be noted that altough PyTorchModelHubMixin can read both the PyTorch's .bin and safetensors formats, it can only write the latter. So any potential future updates to the model would migrate the format automatically to safetensors.

How Has This Been Tested?

Tests have been added in tests/test_model.py that verify that models can be serialized and unserialized without any changes to the parameters or hyperparameters. These fixes passes all tests.

Additionally, I have manually verified that predictions on random data are identical to those produced by the previous version.

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

(I've attempted to follow OCF's coding style, but running black causes many changes in existing code that is not touched by this PR)

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.

1 participant