Skip to content

Modified OpenELM.py, mincpm.py, gpt2.py, gpt_bigcode.py, internlm2.py code to make it work with mypy #61

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
Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Modified OpenELM.py, mincpm.py, gpt2.py, gpt_bigcode.py, internlm2.py code to make it work with mypy #61

wants to merge 24 commits into from

Conversation

ParamThakkar123
Copy link

on executing the following command before this change:
mypy --explicit-package-bases --ignore-missing-imports models/openelm.py

Output:

models\cache.py:50: error: Incompatible types in assignment (expression has type "dict[Any, Any]", variable has type "list[Any]")  [assignment]
models\cache.py:53: error: Incompatible types in assignment (expression has type "dict[Any, Any]", variable has type "list[Collection[Any]]")  [assignment]
models\cache.py:105: error: Incompatible return value type (got "int", expected "list[Any]")  [return-value]
models\openelm.py:49: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "float | int | None"  [type-var]
models\openelm.py:49: error: Unsupported operand types for // ("int" and "None")  [operator]
models\openelm.py:49: note: Right operand is of type "int | None"
models\openelm.py:49: error: Unsupported operand types for * ("int" and "None")  [operator]
models\openelm.py:49: error: Unsupported operand types for / ("None" and "int")  [operator]
models\openelm.py:49: note: Left operand is of type "int | None"
models\openelm.py:49: note: Left operand is of type "float | int"
models\openelm.py:51: error: Unsupported operand types for > ("float" and "None")  [operator]
models\openelm.py:51: note: Left operand is of type "float | int | None"
models\openelm.py:52: error: Unsupported operand types for + ("float" and "None")  [operator]
models\openelm.py:52: error: Unsupported operand types for + ("int" and "None")  [operator]
models\openelm.py:52: error: Unsupported operand types for + ("None" and "int")  [operator]
models\openelm.py:52: error: Unsupported left operand type for + ("None")  [operator]
models\openelm.py:52: note: Both left and right operands are unions
models\openelm.py:53: error: Incompatible return value type (got "float | int | None", expected "float | int")  [return-value]
Found 13 errors in 2 files (checked 1 source file)

After this change:

Success: no issues found in 1 source file

@ParamThakkar123 ParamThakkar123 mentioned this pull request Mar 30, 2025
Open
@ParamThakkar123 ParamThakkar123 changed the title Modified OpenELM.py code to make it work with mypy Modified OpenELM.py, mincpm.py, gpt2.py, gpt_bigcode.py, internlm2.py code to make it work with mypy Apr 1, 2025
awni
awni reviewed Apr 2, 2025
View reviewed changes
awni
awni reviewed Apr 2, 2025
View reviewed changes
@ParamThakkar123 ParamThakkar123 requested a review from awni April 3, 2025 18:32
@ParamThakkar123
Copy link
Author

@awni can you please review this PR ??

@ParamThakkar123
Copy link
Author

@awni can you please review this PR ??

@awni
Copy link
Member

awni commented Apr 11, 2025

Will try and get to it soon.

@ParamThakkar123
Copy link
Author

Sure. Will be waiting for your response. Meanwhile I will try to run the mypy command on the entire repository to see if there are any files which are still incompatible

awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
awni
awni reviewed Apr 11, 2025
View reviewed changes
@ParamThakkar123
Copy link
Author

Thanks for your awesome review @awni ! I would take a note of all the changes and make those as soon as possible

awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni reviewed Apr 18, 2025
View reviewed changes
awni
awni requested changes Apr 18, 2025
View reviewed changes
Copy link
Member

@awni awni left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this. I left a few comments, please address and then I can review again.

@ParamThakkar123
Copy link
Author

@awni Thanks for the amazing and systematic review on this code. I have taken all of your suggestions into considerations and made the changes to this PR

@ParamThakkar123 ParamThakkar123 requested a review from awni April 18, 2025 18:46
awni
awni reviewed Apr 21, 2025
View reviewed changes
mlx_lm/gguf.py
@@ -77,12 +77,14 @@ def get_token_score(self, token_id: int) -> float:
def added_tokens(self) -> Iterable[Tuple[bytes, float, TokenType]]:
for text in self.added_tokens_list:
if text in self.specials:
toktype = self.get_token_type(self.specials[text], "", self.special_ids)
toktype = self.get_token_type(
self.specials[text], text.encode("utf-8"), self.special_ids
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like the same behavior. I'm just wondering what motivated the change here and if this code is still working? Or maybe it wasn't working before?

Copy link
Author

Choose a reason for hiding this comment

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

it gave some mypy errors initially. Doing this change fixed them.

awni
awni reviewed Apr 21, 2025
View reviewed changes
mlx_lm/models/deepseek_v2.py Outdated
@@ -197,54 +198,13 @@ def __init__(self, config: ModelArgs):
**rope_kwargs,
)

def __call__(
Copy link
Member

Choose a reason for hiding this comment

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

What's happening here?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure. How it got deleted. Wasn't intentional. Probably while fixing merge conflicts maybe? Just guessing

awni
awni reviewed Apr 21, 2025
View reviewed changes
awni
awni reviewed Apr 21, 2025
View reviewed changes
awni
awni reviewed Apr 21, 2025
View reviewed changes
awni
awni reviewed Apr 21, 2025
View reviewed changes
awni
awni reviewed Apr 21, 2025
View reviewed changes
awni
awni reviewed Apr 21, 2025
View reviewed changes
awni
awni requested changes Apr 21, 2025
View reviewed changes
Copy link
Member

@awni awni left a comment

Choose a reason for hiding this comment

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

Almost there, I left a few more comments to sort out.

@ParamThakkar123
Copy link
Author

Thanks for your amazing review @awni I will make these changes and push the code today

@ParamThakkar123
Copy link
Author

@awni I made all the changes suggested. Can you please review this??

@ParamThakkar123 ParamThakkar123 requested a review from awni April 26, 2025 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awni awni Awaiting requested review from awni

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.
2 participants
@ParamThakkar123 @awni