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.

Sign up for GitHub

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

Jump to bottom

#13396: Add data parallel support for distilbert model #13158

Merged
merged 1 commit into from
Dec 12, 2024
Merged

#13396: Add data parallel support for distilbert model #13158

merged 1 commit into from
Dec 12, 2024

Conversation

Sudharsan-V
Copy link
Contributor

@Sudharsan-V Sudharsan-V commented Sep 26, 2024 •
edited
Loading

Problem description

The DistilBERT model is configured to run on either N150 or N300, depending on the available machine.

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch 2 times, most recently from df61606 to 974b2d8 Compare September 27, 2024 15:35
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch from 974b2d8 to 5861384 Compare September 30, 2024 16:51
@Sudharsan-V Sudharsan-V marked this pull request as ready for review October 1, 2024 10:56
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch from 5861384 to 4fa79e0 Compare October 1, 2024 11:34
@Sudharsan-V
Copy link
Contributor Author

All post-commit tests : Link (Passed)
Nightly fast dispatch tests : Link (unet and mamba failed)
(Single-card) Demo tests : link (in progress)
(Single-card) Device perf regressions : Link (in progress)
(Single-card) Model perf tests : Link (in progress)

tt-rkim
tt-rkim approved these changes Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@tt-rkim tt-rkim left a comment

Choose a reason for hiding this comment

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

Looks good, but please fix the device perf threshold. It's off

ayerofieiev-tt
ayerofieiev-tt approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@ayerofieiev-tt ayerofieiev-tt left a comment

Choose a reason for hiding this comment

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

not reviewing*

@Sudharsan-V
Copy link
Contributor Author

Looks good, but please fix the device perf threshold. It's off

Sure.

@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch from 5455398 to 1cb1f79 Compare November 28, 2024 08:29
@Sudharsan-V
Copy link
Contributor Author

@uaydonat, @tt-rkim
Please find the status of the latest CIs for this PR:

  • All post-commit tests : Link (Passed)
  • Nightly fast dispatch tests : Link (unet and mamba failed)
  • (Single-card) Demo tests : link (Passed)
  • (Single-card) Device perf regressions : Link (Passed)
  • (Single-card) Model perf tests : Link (Falcon failed on GS, retriggered the CI)

@tt-rkim
Copy link
Collaborator

tt-rkim commented Nov 28, 2024

Looks good. I approved.

uaydonat
uaydonat requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@uaydonat uaydonat left a comment

Choose a reason for hiding this comment

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

please do not merge until you address my questions

models/demos/wormhole/distilbert/README.md Show resolved Hide resolved
models/demos/wormhole/distilbert/tests/test_perf_distilbert.py Outdated Show resolved Hide resolved
tests/scripts/run_performance.sh Show resolved Hide resolved
tests/scripts/run_performance.sh Show resolved Hide resolved
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch from 1cb1f79 to 024e44f Compare November 29, 2024 11:28
@saichandax
Copy link
Contributor

please do not merge until you address my questions

Sure @uaydonat, Sudharsan has responded to it.
We wanted to inform that we received the comments just today, though the review comments are shown as a few days ago. Please note.

@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch from 024e44f to 44f4322 Compare December 3, 2024 08:51
@Sudharsan-V Sudharsan-V requested a review from a team as a code owner December 3, 2024 08:51
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch 2 times, most recently from 6135e0b to fc6e4be Compare December 3, 2024 15:50
@uaydonat uaydonat self-requested a review December 4, 2024 02:20
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch 2 times, most recently from 13cf49e to 6b1f98f Compare December 6, 2024 08:13
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch 2 times, most recently from 243e94f to e45edd0 Compare December 9, 2024 12:57
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch 2 times, most recently from d4b8ed4 to 71badf5 Compare December 11, 2024 09:00
@Sudharsan-V Sudharsan-V force-pushed the sudharsan/ttnn_distilbert_data_parallel branch from 71badf5 to 33c159d Compare December 12, 2024 04:45
@Sudharsan-V
Copy link
Contributor Author

The CIs are passing
(Single-card) Tests for new models
All post-commit tests

Merging the PR with main.

@Sudharsan-V Sudharsan-V merged commit 5d9f7cc into main Dec 12, 2024
162 checks passed
@Sudharsan-V Sudharsan-V deleted the sudharsan/ttnn_distilbert_data_parallel branch December 12, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blozano-tt blozano-tt blozano-tt approved these changes

@ayerofieiev-tt ayerofieiev-tt ayerofieiev-tt approved these changes

@uaydonat uaydonat uaydonat approved these changes

@tt-rkim tt-rkim tt-rkim approved these changes

@boris-drazic boris-drazic boris-drazic approved these changes

@eyonland eyonland Awaiting requested review from eyonland

@dmakoviichuk-tt dmakoviichuk-tt Awaiting requested review from dmakoviichuk-tt dmakoviichuk-tt is a code owner

@rfurko-tt rfurko-tt Awaiting requested review from rfurko-tt rfurko-tt is a code owner

@cfjchu cfjchu Awaiting requested review from cfjchu cfjchu is a code owner

@TT-BrianLiu TT-BrianLiu Awaiting requested review from TT-BrianLiu TT-BrianLiu is a code owner

@razorback3 razorback3 Awaiting requested review from razorback3 razorback3 is a code owner

@dongjin-na dongjin-na Awaiting requested review from dongjin-na dongjin-na is a code owner

@SeanNijjar SeanNijjar Awaiting requested review from SeanNijjar

@yieldthought yieldthought Awaiting requested review from yieldthought

@bbradelTT bbradelTT Awaiting requested review from bbradelTT bbradelTT is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Sudharsan-V @saichandax @uaydonat @tt-rkim @yieldthought @boris-drazic @ayerofieiev-tt @blozano-tt