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

#14732: add bert-tiny test_performance using trace and 2cq-WIP #14799

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#14732: add bert-tiny test_performance using trace and 2cq-WIP #14799

wants to merge 1 commit into from

Conversation

vigneshkeerthivasanx
Copy link
Contributor

Ticket

Link to Github Issue

Problem description

Add trace+2cq test perf file for BERT-Tiny model.

What's changed

Added test perf using trace+2cq file for BERT-Tiny model.

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

@vigneshkeerthivasanx vigneshkeerthivasanx marked this pull request as draft November 6, 2024 13:18
This was referenced Nov 6, 2024
Open
Open
@vigneshkeerthivasanx vigneshkeerthivasanx force-pushed the vignesh/bert_tiny_trace_2cq branch 2 times, most recently from 2ea75aa to 5b8d6d6 Compare November 11, 2024 06:33
@vigneshkeerthivasanx vigneshkeerthivasanx marked this pull request as ready for review November 11, 2024 07:05
@vigneshkeerthivasanx
Copy link
Contributor Author

vigneshkeerthivasanx commented Nov 11, 2024 •
edited
Loading

@mbahnasTT ,
Trace+2cq test performance for the BERT-Tiny model is complete. Bert-Tiny model doesn't support sharded inputs and implemented for the unsharded input tensors. Could I get a review for this pipeline?

ayerofieiev-tt
ayerofieiev-tt approved these changes Nov 14, 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.

approving for codeowners only, not reviewing this PR

@uaydonat
Copy link
Contributor

Is this a superset of #13471? If so, should we just combine the PRs into just one?

@uaydonat
Copy link
Contributor

@esmalTT can you review the 2CQ implementation?

@vigneshkeerthivasanx
Copy link
Contributor Author

Is this a superset of #13471? If so, should we just combine the PRs into just one?

We're awaiting the merge of #13471 first and will rebase afterward. We didn't want #13471 to get held up due to trace+2cq implementation.

@saichandax
Copy link
Contributor

@esmalTT can you review the 2CQ implementation?

Added @esmalTT as reviewer, to notify. Please note.

@tt-rkim
Copy link
Collaborator

tt-rkim commented Nov 18, 2024

No CI runs, at all? Again?

Screenshot 2024-11-18 at 10 48 33 AM

@saichandax
Copy link
Contributor

@tt-rkim,
Trace_2cqs perf implementation hasn't been done before by MCW. This is the first model for which we have done trace+2cqs successfully. So, we are waiting for any review comments.
We'll trigger CIs if there are no changes requested.

We didn't want to trigger without any approval on this PR, specifically. Is that fine?

@tt-rkim
Copy link
Collaborator

tt-rkim commented Nov 18, 2024

You won't get my approval without passing CI.

This is fine, but then I will report back that CI is not the blocker.

esmalTT
esmalTT requested changes Nov 19, 2024
View reviewed changes
models/demos/bert_tiny/tests/test_performance.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we running this test in CI?

models/demos/bert_tiny/tests/test_perf_e2e_bert_tiny.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we running this test in CI?

models/demos/bert_tiny/tests/perf_e2e_bert_tiny.py Outdated

profiler.start("cache")
ttnn.wait_for_event(1, op_event)
# ttnn.copy_host_to_device_tensor(tt_inputs_host, tt_input_ids, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't commit commented code.

I won't bother commenting on every single line, but there are a bunch of places in this PR where you need to get rid of the commented code.

models/demos/bert_tiny/tests/perf_e2e_bert_tiny.py
ttnn.record_event(1, write_event)
ttnn.wait_for_event(0, write_event)
# TODO: Add in place support to ttnn to_memory_config
# input_tensor = ttnn.reshard(tt_input_ids, input_mem_config, input_tensor)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be uncommented to work properly

models/demos/bert_tiny/tests/perf_e2e_bert_tiny.py
signpost(header="stop")
ttnn.dump_device_profiler(device)

ttnn.release_trace(device, tid)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably have some asserts (like checking PCC) in here to make sure things are working as expected.

@esmalTT
Copy link
Contributor

esmalTT commented Nov 19, 2024

@vigneshkeerthivasanx Please fill out the checklist section with links to CI runs before requesting reviews. There is no point in reviewing broken code.

@uaydonat
Copy link
Contributor

Did you run the posted code at least locally and confirmed that it works?

If so, it is safe to launch the CI, see that your tests pass, then post the passing CI links.

Then, we review the code. As Evan pointed out, it will waste of time to review untested broken code.

@vigneshkeerthivasanx
Copy link
Contributor Author

Will update the code and trigger CIs.

@vigneshkeerthivasanx
Copy link
Contributor Author

CI Links
All post-commit tests: Link (Bert-tiny is passing)
Nightly fast dispatch tests: Link (Bert-tiny is passing)
(Single-card) Demo tests: Link (Bert-tiny is passing)
(Single-card) Device perf regressions: Link (Bert-tiny is passing)
(Single-card) Model perf tests : Link (Bert-tiny is passing)

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

@esmalTT esmalTT esmalTT requested changes

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

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

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

@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

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
6 participants
@vigneshkeerthivasanx @uaydonat @saichandax @tt-rkim @esmalTT @ayerofieiev-tt