Skip to content

Conversation

@Yacklin
Copy link
Contributor

@Yacklin Yacklin commented Nov 26, 2025

What does this PR do?

When using kimi model, kimi developers used something like "bool|int" as type hint for parameters in function signature and unfortunately, due to python's nature, format of type hint like "bool|int" is not completely the same as "typing.Union[bool, int]" until python 3.14. One of the biggest differences is when accessing param.annotation.__name__, "bool|int" would raise error that has no attribute "__name__". This error bothers me a lot so i have to fix it. I found a beautiful way to fix it and its backward compatibility could be extended to 3.4 (I just tried it a little bit but you could try versions before 3.4).

Please be advised that param_name and func are unused in this rewrited version. I chose not to remove these two parameters just in case something bad would happen.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@Rocketknight1
(I accidentally deleted the previous PR, here is the new one)

@Rocketknight1
Copy link
Member

Approved the doc build run! You should get a link to the docs with your PR in about 30 minutes - can you check that everything is working as expected?

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 28, 2025

Approved the doc build run! You should get a link to the docs with your PR in about 30 minutes - can you check that everything is working as expected?

i think yes. Could you please let me know if anything should be changed in my code for this PR? I tested this rewritten fuction several times as well. Also, i am a little bit new to git and github so sometimes i might mess up PR or commits. Sorry for that in advance. @Rocketknight1

@Yacklin Yacklin force-pushed the rewrite-_process_parameter_type-to-improve-usability branch from bf593b7 to f951d0c Compare November 28, 2025 21:07
@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 28, 2025

i am wondering why sometimes tests failed but sometimes didn't? And my changes have nothing to do with these failures

@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 28, 2025

Approved the doc build run! You should get a link to the docs with your PR in about 30 minutes - can you check that everything is working as expected?

Okay i do find something changed, which i would prefer actually.
current one:
image
updated one:
image

Might be a little bit verbose but i think it's okay

@Yacklin
Copy link
Contributor Author

Yacklin commented Nov 28, 2025

Maybe i should provide more details about this PR.
When using kimi 48B instruct model, the decorator @auto_docstring would be applied to some functions in modelling_kimi like this:
image

But, auto_docstring would call _process_parameter_type to go through every single parameter's type hint to call this type hint's annotation.__name__. But before python 3.14, bool|int is not the same as Union[bool, int]. It means type hint like "bool|hint" has no attribute __name__. Then _process_parameter_type would raise errors, which prevents users from using kimi 48B instruct models.
image

Btw, the downstream users of kimi 48B noticed this problem then they changed it but it seems that they didn't notify kimi this problem:

image (coming from modelling_kimi.py of cerebras/Kimi-Linear-REAP-35B-A3B-Instruct)

My PR could solve this problem. Otherwise, python 3.14 is needed to use kimi 48B instruct model and i don't think transformers users could switch to python 3.14 easily
@Rocketknight1

@Yacklin Yacklin force-pushed the rewrite-_process_parameter_type-to-improve-usability branch from f951d0c to 98d26ea Compare November 30, 2025 18:52
Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

Hey @Yacklin, I took a look at the code! I think the bug you're fixing is real, and I think the cause is that we do lots of hacky accesses to things like annotation.__name__, like you said. I don't like if ":" in parameter_str though - doing these kinds of string manipulations also feels very hacky. It's likely to create bugs in future if anything changes with typing, or if anyone writes an unusual type hint.

Is there a clean way to correctly resolve all type hints into some kind of canonical form, and then just operate on that, rather than relying on string representations and attribute accesses?

@Yacklin
Copy link
Contributor Author

Yacklin commented Dec 2, 2025

Hey @Yacklin, I took a look at the code! I think the bug you're fixing is real, and I think the cause is that we do lots of hacky accesses to things like annotation.__name__, like you said. I don't like if ":" in parameter_str though - doing these kinds of string manipulations also feels very hacky. It's likely to create bugs in future if anything changes with typing, or if anyone writes an unusual type hint.

Is there a clean way to correctly resolve all type hints into some kind of canonical form, and then just operate on that, rather than relying on string representations and attribute accesses?

Hi @Rocketknight1 , string manipulation is unavoidable in this function because this function would return something like tuple[str, bool]. But i could try to write an implmentation to minimize string manipulation.

@Rocketknight1
Copy link
Member

Hi @Yacklin, some manipulation is unavoidable at the end, but I think you should always be able to get the type hint from param.annotation rather than needing to use str(param). What we really want to do is just grab that annotation in the safest and most Pythonic way possible and turn it into a "canonical" form regardless of whether it was written as Optional[Union[str, bool]] or str | bool | None. I don't think flattening it to a string is the safest way to do that, and will probably get very messy for nested types!

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