search

tenstorrent / tt-mlir

Tenstorrent MLIR compiler
https://tenstorrent.github.io/tt-mlir/
Apache License 2.0
51 stars 7 forks source link

Revert previously reverted changes + add fix #464

Closed svuckovicTT closed 3 weeks ago

svuckovicTT commented 3 weeks ago
tapspatel commented 3 weeks ago

thanks so much @svuckovicTT !!! lgtm

svuckovicTT commented 3 weeks ago

I'd prefer to merge this WITHOUT squashing. Since there's multiple unrelated commits in this change, it'll be easier for someone to revert/cherry-pick a specific commit down the road, if there's a need. Lmk if you have any objections to that.

nsmithtt commented 3 weeks ago

I'd prefer to merge this WITHOUT squashing. Since there's multiple unrelated commits in this change, it'll be easier for someone to revert/cherry-pick a specific commit down the road, if there's a need. Lmk if you have any objections to that.

Perhaps we can do them as separate PRs then? The reason for the hard enforcement of squash is that it drastically simplifies the git history and CI pipeline history, on main. It enables someone to checkout any commit on main and build and run and pass all unit tests. If we don't' enforce squashing then there are gaps, this is bad for 2 reasons:

There's definitely tradeoffs with this approach, but having this invariant is extremely powerful and usually outweighs the downsides. I'd also argue that this method makes it easier to revert or cherry-pick, there are very few cases in my mind where you'd want to only cherry-pick half of a change, if a change could have been broken into pieces it probably should have been separate PRs to begin with.

svuckovicTT commented 3 weeks ago

I'd prefer to merge this WITHOUT squashing. Since there's multiple unrelated commits in this change, it'll be easier for someone to revert/cherry-pick a specific commit down the road, if there's a need. Lmk if you have any objections to that.

Perhaps we can do them as separate PRs then? The reason for the hard enforcement of squash is that it drastically simplifies the git history and CI pipeline history, on main. It enables someone to checkout any commit on main and build and run and pass all unit tests. If we don't' enforce squashing then there are gaps, this is bad for 2 reasons:

  • Someone could checkout some commit on main that's in an undefined state, it's just in the middle of some larger change, may or might not pass unit tests, who knows.
  • Unable to generically use git bisect for regression testing. If every commit is expected to work and tested to some degree of confidence, then git bisect can freely walk the tree and have a fully automated bisect.

There's definitely tradeoffs with this approach, but having this invariant is extremely powerful and usually outweighs the downsides. I'd also argue that this method makes it easier to revert or cherry-pick, there are very few cases in my mind where you'd want to only cherry-pick half of a change, if a change could have been broken into pieces it probably should have been separate PRs to begin with.

Ah, I didn't realize that squashing is enforced - I see your points though, and agree that it's better to squash.

Luckily, the PR will remain, and the commits in the PR itself will remain separate so if anyone needs to cherry pick anything, they can.