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

[tuner] Add candidate generator script #141

Merged
merged 24 commits into from
Aug 28, 2024
Merged

Conversation

RattataKing
Copy link
Contributor

@RattataKing RattataKing commented Aug 22, 2024

The plan is to move the tuner from the sdxl-scripts repo to the sharktank repo.
This PR is the first step, moving candidate_gen.py (the candidate generator for tuning) along with its CI and unit tests.
More changes will come to fully decouple it.

New changes in candidate_gen.py:

  • Edited comments
  • Added a registry class for dispatch kind
  • Moved dispatch kind specific functions from global to their belonged classes
  • Changed how it walks op and when it decides which functions to use for candidate generation.
  • Checked if input mlir translation info contains LLVMGPUVectorDistribute

candidate_gen_test.py:

  • No new tests added, updated the existing function to match changes in candidate_gen.py

@RattataKing RattataKing requested a review from kuhar August 22, 2024 17:58
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Please add a description to this PR and say what the plan is wrt landing the tuner.

Also please include the unit tests together with the code.

@kuhar kuhar requested a review from ScottTodd August 22, 2024 18:47
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

A few comments from an initial scan. Definitely want to resolve the github workflow / requirement file topics before merge here. Could defer the script refactoring.

tuner-requirements.txt Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
@RattataKing RattataKing force-pushed the add_tuner branch 2 times, most recently from c5c8085 to 2bedfdc Compare August 23, 2024 16:27
@RattataKing RattataKing requested review from kuhar and ScottTodd August 23, 2024 22:42
.github/workflows/ci-tuner.yml Outdated Show resolved Hide resolved
.github/workflows/ci-tuner.yml Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/requirements-dev.txt Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/requirements-tuner.txt Outdated Show resolved Hide resolved
@RattataKing RattataKing requested a review from ScottTodd August 23, 2024 22:55
sharktank/tests/tuner/candidate_gen_test.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
@RattataKing RattataKing requested review from ScottTodd and kuhar August 27, 2024 16:42
Copy link
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

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

A README.md in sharktank/tools/tuner/ would be helpful too

sharktank/sharktank/tools/tuner/candidate_gen.py Outdated Show resolved Hide resolved
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the cleanups

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

The new location looks good to me

@kuhar kuhar merged commit 89cc4c5 into nod-ai:main Aug 28, 2024
6 of 7 checks passed
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