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

Huggingface hub integration 5545 #5930

Merged
merged 43 commits into from
Feb 4, 2023

Conversation

wwymak
Copy link
Contributor

@wwymak wwymak commented Nov 8, 2022

This is an initial draft of what the wrapper could look like.

Some questions I have:

  • is this a sensible approach? (I have overridden the from_pretrained and save_pretrained methods so we can have defaults for the model cards, and a slightly different way of initialising the PyG model as some of them require the data at init time)
  • should the wrapper live somewhere else in the codebase?
  • do we want to have a nicer template for PyG models or just use the default one? Are there fields we want to have prepopulated by default
  • should the wrapper also contain methods to log the training/evaluation metrics by default?
    @rusty1s would be great if you comment :)

The screenshot is an example of what it currently looks like if you use the wrapper to upload to huggingface model hub
Screenshot from 2022-11-08 22-08-56

@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #5930 (d96149a) into master (c0b10c0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head d96149a differs from pull request most recent head ad7f405. Consider uploading reports for the commit ad7f405 to get more accurate results

@@            Coverage Diff             @@
##           master    #5930      +/-   ##
==========================================
+ Coverage   87.16%   87.18%   +0.02%     
==========================================
  Files         418      419       +1     
  Lines       22789    22838      +49     
==========================================
+ Hits        19863    19912      +49     
  Misses       2926     2926              
Impacted Files Coverage Δ
torch_geometric/nn/model_hub.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rusty1s
Copy link
Member

rusty1s commented Nov 9, 2022

This looks great. Happy to fine-tune it in later PRs as well.

is this a sensible approach? (I have overridden the from_pretrained and save_pretrained methods so we can have defaults for the model cards, and a slightly different way of initialising the PyG model as some of them require the data at init time)

I think so. Currently, we are creating a new model while I would prefer something like SchNet(ModelHubMixin).

should the wrapper live somewhere else in the codebase?

I think it should live in torch_geometric.nn rather than torch_geometric.nn.models.

do we want to have a nicer template for PyG models or just use the default one? Are there fields we want to have prepopulated by default

I think the default one works for now. Happy to fine-tune it in later PRs if that is possible. Please feel free to get creative :)

should the wrapper also contain methods to log the training/evaluation metrics by default?

I am not sure. Let's go with a basic version first.

@wwymak wwymak requested a review from rusty1s November 14, 2022 22:53
@wwymak
Copy link
Contributor Author

wwymak commented Nov 14, 2022

Hi @rusty1s I have converted the integration to a mixin, and added an example -- is this more along the lines of what you are thinking of?

@rusty1s
Copy link
Member

rusty1s commented Nov 15, 2022

Thanks, this is super cool. Wondering how we can integrate a basic test for this. Any idea?

@wwymak
Copy link
Contributor Author

wwymak commented Nov 15, 2022

Wondering how we can integrate a basic test for this. Any idea?

I am thinking of a simple version of https://github.com/huggingface/huggingface_hub/blob/main/tests/test_hubmixin.py ? Since ours is based on what they've done for the pytorch mixin?

@wwymak wwymak requested review from wsad1 and removed request for rusty1s February 3, 2023 22:39
Copy link
Member

@wsad1 wsad1 left a comment

Choose a reason for hiding this comment

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

Thanks again @wwymak for the great work. Will merge after tests pass.

@wsad1 wsad1 enabled auto-merge (squash) February 4, 2023 11:30
@wsad1 wsad1 merged commit 3139c48 into pyg-team:master Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants