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

Enable MPS support for LitGPT #1724

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Enable MPS support for LitGPT #1724

merged 5 commits into from
Sep 13, 2024

Conversation

rasbt
Copy link
Collaborator

@rasbt rasbt commented Sep 12, 2024

Re-enables MPS support for LitGPT with an alternative index_copy_ implementation.

@rasbt rasbt merged commit e570612 into main Sep 13, 2024
9 checks passed
@rasbt rasbt deleted the reenable-mps-support branch September 13, 2024 02:11
result = x.new_zeros(*x.shape[:-1], self.linear.out_features) # (64, 64, 384)
return result.index_copy_(dim=-1, index=self.lora_ind, source=x) # (64, 64, 384)
else:
if all(self.enable_lora):
Copy link
Collaborator

Choose a reason for hiding this comment

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

That shouldn't be in the compatibility mode.
In fact, only index copy should be altered with slicing in the compatibility mode.

@t-vi
Copy link
Contributor

t-vi commented Sep 13, 2024

To my mind, the implementation should not wire a mps compatibility mode through the model but just check the tensor device in index copy.
This is quite a layering leak and does not look like good UX.
If we're concerned about testing, we should bump the litgpt ci to apple silicon or have a function for the mps impl that we test separately.

@Andrei-Aksionov
Copy link
Collaborator

just check the tensor device in index copy.

Agree.

If we're concerned about testing, we should bump the litgpt ci to apple silicon or have a function for the mps impl that we test separately.

M1 runner for public repos should be free of charge: actions/runner-images#9254

@t-vi
Copy link
Contributor

t-vi commented Sep 13, 2024

Yeah, we need to switch to M1 anyways because x86 macs don't get PyTorch support anymore (I think the last version was 2.2).

@t-vi t-vi mentioned this pull request Sep 13, 2024
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.

3 participants