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

[Vicuna] Revert the formatting for Brevitas op #1626

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

Abhishek-Varma
Copy link
Contributor

-- This commit reverts the formatting for Brevitas op.
-- It also excludes vicuna.py script from black formatter.

Signed-off-by: Abhishek Varma [email protected]

-- This commit reverts the formatting for Brevitas op.
-- It also excludes vicuna.py script from `black` formatter.

Signed-off-by: Abhishek Varma <[email protected]>
rhs_bit_width: int,
rhs_group_size: int,
) -> List[int]:
def brevitas〇matmul_rhs_group_quant〡shape(lhs: List[int], rhs: List[int], rhs_scale: List[int], rhs_zero_point: List[int], rhs_bit_width: int, rhs_group_size: int) -> List[int]:
Copy link
Contributor

Choose a reason for hiding this comment

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

@stellaraccident do you know how this changes the function signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

And @makslevental any thoughts

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is because of the way these functions are verified in Torch-MLIR. They match the functions' first line for verifying the signature (https://github.com/llvm/torch-mlir/blob/main/python/torch_mlir/dialects/torch/importer/jit_ir/build_tools/library_generator.py#L177), and after formatting the function definition goes to multiple lines and as a result, it would fail the verification.

@powderluv
Copy link
Contributor

@Abhishek-Varma but why revert the other files

Copy link
Contributor

@powderluv powderluv left a comment

Choose a reason for hiding this comment

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

Why the changes the pyproject too?

@Abhishek-Varma
Copy link
Contributor Author

Hi @powderluv

I wasn't including the other files in this patch initially but the CI showed linting failures in those set of files - opt_params and model_wrappers.py.
So I applied black formatting on those as well and added them.

Also, pyproject was changed to exclude vicuna.py from being formatted by black - this is with reference to Jinchen's comment here and also because of the formatting it doesn't get registered and fails the lowering at torch-mlir level itself. CC: @vivekkhandelwal1

@powderluv
Copy link
Contributor

Opened #1627 to track this issue. so we can unblock the demos

@powderluv powderluv merged commit 1b62dc4 into nod-ai:main Jul 6, 2023
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