Skip to content

Fix torch.clamp issue #237#238

Open
CryptoSalamander wants to merge 1 commit intomicrosoft:mainfrom
CryptoSalamander:fix/clamp-issue
Open

Fix torch.clamp issue #237#238
CryptoSalamander wants to merge 1 commit intomicrosoft:mainfrom
CryptoSalamander:fix/clamp-issue

Conversation

@CryptoSalamander
Copy link

@CryptoSalamander CryptoSalamander commented Jul 14, 2022

This PR is related to #237 !
There are two options to fix this problem,

  1. just convert max tensor to scalar (this PR)
  2. make max tensor be loaded the same device with self.logit_scale like below:
device = self.logit_scale.device
max_tensor = torch.log(torch.tensor(1. / 0.01)).to(device)
logit_scale = torch.clamp(self.logit_scale, max=max_tensor).exp()

I think the first option is better due to its simplicity.
I tested both options on my datasets, it seems no difference in cuda memory allocation & inference speed.

@CryptoSalamander
Copy link
Author

CryptoSalamander commented Jul 14, 2022

@ancientmooner Could you please check issue #237 ?

@juncgu
Copy link

juncgu commented Apr 11, 2023

@CryptoSalamander, I would prefer to use the second option. I faced the same issue when using torch 2.0, and the item() method in the first option will lead torch.dynamo to break WindowAttention into two graphs when tracing the module.

@CryptoSalamander CryptoSalamander force-pushed the fix/clamp-issue branch 3 times, most recently from ad49644 to 38eba56 Compare April 11, 2023 13:57
@CryptoSalamander
Copy link
Author

CryptoSalamander commented Apr 11, 2023 •
edited
Loading

@juncgu Thanks for your suggestion. I have modified the code as the second option.
Could you please take a look at this PR? @ancientmooner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@CryptoSalamander @juncgu