Skip to content

Change the transformer to norm_first by default - #599

Merged
angeloskath merged 1 commit into
mainfrom
update-transformer
Jan 31, 2024
Merged

angeloskath merged 1 commit into
mainfrom
update-transformer

Conversation

@angeloskath

Copy link
Copy Markdown
Member

This is a backwards incompatible change as it changes the default to norm_first however I think this should have always been the default and hopefully if not too many people relied on it we caught it early.

If not we can cancel this PR.

@angeloskath
angeloskath requested review from awni and jagrit06 January 31, 2024 07:44

@awni awni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yea at some point I reviewed a PR which added a bunch of stuff to Transformer including this. I think I said we should just leave it to post norm since it's way more common now (?).

But this gets us compatibility with PyTorch so I'm also on board switching. I don't think it should break too much. In most of our examples we reimplement the Transformer anyway 😅

@angeloskath

Copy link
Copy Markdown
Member Author

Just to be clear. This breaks compatibility with PT and moves to the more common option. As you said the only thing that is used in places is the attention implementation and I think this is more of a guide/example at this point.

@angeloskath
angeloskath merged commit ba8d6bf into main Jan 31, 2024
@angeloskath
angeloskath deleted the update-transformer branch January 31, 2024 20:55
@awni

awni commented Jan 31, 2024

Copy link
Copy Markdown
Member

Sorry I confused myself 😅 , I actually did the exact opposite of what I thought I did in this PR #167 , where it was switched to post norm by default.

I am fine with reverting that.

@awni

awni commented Jan 31, 2024

Copy link
Copy Markdown
Member

@angeloskath feel free to merge this. I tend to prefer supporting the more common option (especially when it is much more common) over perfect compatibility with PyTorch

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.

2 participants