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.

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

Adding Mock Model + Test Directory #24

Merged

Conversation

mvanniasingheTT
Copy link
Contributor

This PR adds in the mock model + mock offline inference script + a new test directory.
The mock model can be used for quick iteration and development without a TT device to test benchmarking/stress-testing/eval procedures in either an online or offline setting.

@milank94
Copy link
Contributor

It's confusing at the moment having two sets of code changes where one is a copy of #21. Hard to see which code pieces you would like the focus of the review on.

My recommendation is either:

tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
@tstescoTT
Copy link
Contributor

It's confusing at the moment having two sets of code changes where one is a copy of #21. Hard to see which code pieces you would like the focus of the review on.

My recommendation is either:

* Change the target branch to `tstesco/evals-benchmarking-structure`

* if targeting `main`, don't include those code changes that are in [evals and benchmarking structure #21](https://github.com/tenstorrent/tt-inference-server/pull/21)

This makes sense, I'll add that to enable making changes that require other unmerged changes on another branch we shouldnt merge onto the other branch as the default but wait until that branch is merged to main then change the new leaf branch to point to main as well. The process would be:

  1. PR for review should start against the other branch for reference (tstesco/evals-benchmarking-structure in this case).
  2. Don't merge PR into the base branch unless desired by the base branch author (to avoid confusion with other PR review).
  3. Once the other branch is merged to main the PR can be switched to target main.

@mvanniasingheTT mvanniasingheTT changed the base branch from main to tstesco/evals-benchmarking-structure October 30, 2024 20:54
@mvanniasingheTT mvanniasingheTT force-pushed the mvanniasinghe/mock_model branch 3 times, most recently from 4dc87cd to bf2e08b Compare October 31, 2024 18:59
tests/README.md Outdated Show resolved Hide resolved
Co-authored-by: Milan Kordic <[email protected]>
tests/README.md Outdated Show resolved Hide resolved
tests/README.md Outdated Show resolved Hide resolved
tests/mock_vllm_model.py Outdated Show resolved Hide resolved
tests/mock_vllm_model.py Outdated Show resolved Hide resolved
@milank94
Copy link
Contributor

milank94 commented Nov 4, 2024

@mvanniasingheTT I think the code could benefit by running a formatter and linter. Consider the Ruff VSCode extension as an easy plugin to run.

CC: @tstescoTT

Copy link
Contributor

@milank94 milank94 left a comment

Choose a reason for hiding this comment

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

Looks great. Pending the merge of #21.

@mvanniasingheTT mvanniasingheTT merged commit 9bb28d0 into tstesco/evals-benchmarking-structure Nov 5, 2024
@milank94
Copy link
Contributor

milank94 commented Nov 5, 2024

@mvanniasingheTT I think that you forgot to change the target branch?

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.

3 participants