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

gpt-bigcode: avoid zero_ to support Core ML #24755

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

pcuenca
Copy link
Member

@pcuenca pcuenca commented Jul 11, 2023

What does this PR do?

In-place zero_ is not supported by the Core ML conversion process. This PR replaces it with zeros_like so conversion can proceed.

The change only affects a workaround for a PyTorch bug on the cpu device.

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@younesbelkada, @loubnabnl, @jlamypoirier

In-place `zeros_` is not supported by the Core ML conversion process.
This PR replaces it with `zeros_like` so conversion can proceed.

The change only affects a workaround for a PyTorch bug on the `cpu`
device.
@pcuenca
Copy link
Member Author

pcuenca commented Jul 11, 2023

Note: to fully test conversion of gpt-bigcode models, the following coremltools PRs (or equivalent workarounds) need to be applied as well: apple/coremltools#1910, apple/coremltools#1911.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jul 11, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@jlamypoirier jlamypoirier left a comment

Choose a reason for hiding this comment

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

I suggest moving the torch.empty to the else to avoid double allocation, otherwise lgtm

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks @pcuenca !

@younesbelkada younesbelkada requested a review from sgugger July 12, 2023 07:14
@mayank31398
Copy link
Contributor

mayank31398 commented Jul 12, 2023

@younesbelkada I think this is already fixed in PT.
Should we just drop this logic?
Opened a PR: #24768 which supercedes this one

@sgugger
Copy link
Collaborator

sgugger commented Jul 12, 2023

We support versions of PyTorch from 1.10 and onward, so we need to keep the workaround for the bug.

@younesbelkada
Copy link
Contributor

Merging to unblock @pcuenca , let's maybe address @jlamypoirier 's comments in a follow up PR !

@younesbelkada younesbelkada merged commit 395e566 into huggingface:main Jul 12, 2023
@pcuenca pcuenca deleted the gpt-bigcode-coreml-support branch July 12, 2023 14:40
Lorenzobattistela pushed a commit to Lorenzobattistela/transformers that referenced this pull request Jul 13, 2023
gpt-bigcode: avoid `zeros_` to support Core ML.

In-place `zeros_` is not supported by the Core ML conversion process.
This PR replaces it with `zeros_like` so conversion can proceed.

The change only affects a workaround for a PyTorch bug on the `cpu`
device.
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
gpt-bigcode: avoid `zeros_` to support Core ML.

In-place `zeros_` is not supported by the Core ML conversion process.
This PR replaces it with `zeros_like` so conversion can proceed.

The change only affects a workaround for a PyTorch bug on the `cpu`
device.
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.

6 participants