Enhance the handling of Union types in HfArgumentParser#41441
Enhance the handling of Union types in HfArgumentParser#41441cyyever wants to merge 2 commits intohuggingface:mainfrom
Conversation
|
@SunMarc This can resolve |
cyyever
changed the title
cyyever
changed the title
Signed-off-by: Yuanyuan Chen <cyyever@outlook.com>
|
CI seems green so gentle ping @SunMarc |
|
@Rocketknight1 @SunMarc We also need it for moving to Python 3.10 |
SunMarc
left a comment
SunMarc
left a comment
There was a problem hiding this comment.
Thanks, this was always been a bit of a mess, I left a comment regarding how to fix this, would you like to fix fixing this ?
| if len(field.type.__args__) > 2: | ||
| origin_type = str | ||
| else: | ||
| # filter `str` in Union | ||
| field.type = field.type.__args__[0] if field.type.__args__[1] is str else field.type.__args__[1] | ||
| origin_type = getattr(field.type, "__origin__", field.type) |
There was a problem hiding this comment.
This is a really special case when None is not in the field. In most cases, this don't happen at all. For example for fsdp, we have dict[str, Any] | str | None. So it is not going here but instead in the
elif bool not in field.type.__args__: condition.
Could you try to clean a bit the mess here ? From what I see:
- If we have bool + None, we don't touch
- if we don't have none, we only allow one class
- if we have none, we can allow more:
3.1 if we have str and other types, we should enforce the user to pass str.
3.2 if we don't have str, we use the first type passed
There was a problem hiding this comment.
If you can add some tests testing those, that would be huge also
3 participants
What does this PR do?
This PR enhance the handling of Union types containing
strby two rules:strinUnionandUnionhas more than one other types passstrtoparser.add_argument. The caller is responsible to convert the string argument to a more proper type.Unionmust beX | strand passXtoparser.add_argument