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

Add integration with HF Hub #7833

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

qubvel
Copy link

@qubvel qubvel commented Jun 10, 2024

Description

Hi! Amazing repo!

I made a draft with PytorchModelHubMixin for a few models in the repo. This mixin adds a few methods to make it possible to save/load models and share them with the HF hub.

Here is a short example:

from monai.networks.nets import AHNet

net = AHNet()

# save to a local folder
net.save_pretrained("qubvel-hf/ahnet")

# push to HF hub
net.push_to_hub("qubvel-hf/ahnet")

# load form local folder or HF hub
net = AHNet.from_pretrained("qubvel-hf/ahnet")

I believe the repo and community may benefit from sharing pretrained models built with this framework!
Let me know if it aligns with your roadmap, I will be happy to help you with implementation or to complete the PR!

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

@qubvel
Copy link
Author

qubvel commented Jun 18, 2024

Hey! I would appreciate any comments on this 🤗
ping @ericspod @KumoLiu for visibility

Comment on lines +24 to +27
try:
from huggingface_hub import PyTorchModelHubMixin
except ImportError:
PyTorchModelHubMixin = DummyPyTorchModelHubMixin
Copy link
Member

Choose a reason for hiding this comment

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

Hi @qubvel Thanks for the contribution. We have our own optional_import function to do something similar.

Suggested change
try:
from huggingface_hub import PyTorchModelHubMixin
except ImportError:
PyTorchModelHubMixin = DummyPyTorchModelHubMixin
PyTorchModelHubMixin, has_hg_hub = optional_import("huggingface_hub", name="PyTorchModelHubMixin")
if not has_hg_hub:
PyTorchModelHubMixin = DummyPyTorchModelHubMixin # could put DummyPyTorchModelHubMixin definition here instead

Copy link
Member

Choose a reason for hiding this comment

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

optional_import provides its own dummy class which raises an exception whenever a member is requested, but this doesn't have __init_subclass__ so your version is needed.

Copy link
Author

Choose a reason for hiding this comment

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

ok, great, I will update it!

@ericspod
Copy link
Member

ericspod commented Jun 18, 2024

Hi @qubvel Thanks again. Please double check places like this in setup.cfg have the updated information as your code mentions needed safetensors. Is it possible to perhaps subclass these networks as well? I am hesitant to modify existing definitions as a general rule even though things like the model weights are unchanged. For example instead of changing AHNet we would have class AHNetHub(AHNet, MonaiHubMixin): pass. I'd like some feedback from others on this as well.

Copy link
Member

Choose a reason for hiding this comment

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

Flake8 is correct in that this file is missing the license header that's in all our other source files.

Copy link
Author

Choose a reason for hiding this comment

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

got it 👍

@qubvel
Copy link
Author

qubvel commented Jun 18, 2024

@ericspod thanks for the review, I will address the comments! At the moment it's a raw draft to know how you feel about this integration in general. Any comments on how to make it smooth are appreciated!

I'm not sure I got the point here, can you provide more details?

Is it possible to perhaps subclass these networks as well? I am hesitant to modify existing definitions as a general rule even though things like the model weights are unchanged.

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.

2 participants