huggingface / community-events

Place where folks can contribute to 🤗 community events
400 stars 100 forks source link

huggan.pytorch.lightweight_gan.lightweight_gan.LightweightGAN _from_pretrained requires use_auth_token but this is not passed by the from_pretrained method inherited from ModelHubMixin #194

Closed johnowhitaker closed 1 year ago

johnowhitaker commented 1 year ago

In the mixin code (https://github.com/huggingface/huggingface_hub/blob/9e0ac58813df4e0414d6fd494040953f053dbe0d/src/huggingface_hub/hub_mixin.py#L93) from_pretrained calls _from_pretrained but doesn't pass in a use_auth_token argument. In the LightweightGAN code this argument is required: https://github.com/huggingface/community-events/blob/main/huggan/pytorch/lightweight_gan/lightweight_gan.py#L854

Notebook showing how this manifests for a user: https://colab.research.google.com/drive/1Lc42pRp0-ZxFKbhfU420ZrpXfA8Q-k-e?usp=sharing (includes how I worked around this for now). Currently if you follow the example usage at e.g. https://huggingface.co/ceyda/butterfly_cropped_uniq1K_512 you'll get an error.

Suggested fix is to just add a default for use_auth_token=None in the LightweightGAN _from_pretrained method, but creating an issue in case someone wants to do a more thorough fix. This code is very rarely used but I've had at least one keen learner stuck on this.

johnowhitaker commented 1 year ago

PR that maybe fixes the issue: https://github.com/huggingface/community-events/pull/195

Wauplin commented 1 year ago

Hi @johnowhitaker, sorry you've experienced this issue and thanks for investigating it :)

We are in the -long- process of removing use_auth_token in favor of token everywhere in the HF ecosystem (instead of having a bit of both which is inconsistent). There is a lot of inertia to this switch because of all the existing code and scripts. What I would suggest here (in #195) is:

And this should definitely solve the issue. LightweightGAN will not be compatible with very old versions of huggingface_hub (<=0.10) but let's not care about that (latest is 0.16.4). The problem with PR #195 currently is that even if it makes the code run, the token is still not passed correctly.

Wauplin commented 1 year ago

For more context, here is the piece of code that handles use_auth_token => token during the transition process. The plan described in the docstring is delayed by far but overall still the plan.

johnowhitaker commented 1 year ago

@Wauplin Thanks for the help! I updated the PR with those suggested changes.

Wauplin commented 1 year ago

Thanks for taking care! I just merged the PR :)