Skip to content

Fix typing order#39467

Merged
SunMarc merged 6 commits intohuggingface:mainfrom
Tavish9:typing
Jul 17, 2025
Merged

Fix typing order#39467
SunMarc merged 6 commits intohuggingface:mainfrom
Tavish9:typing

Conversation

@Tavish9
Copy link
Copy Markdown
Contributor

@Tavish9 Tavish9 commented Jul 17, 2025

What does this PR do?

Fixes #39462

Importing order matters
Before #38797, Union[str, Dict] was okay for type Union[dict, str].
When changing from Dict from dict, Union[str, dict] would predominate Union[dict, str] if it's imported early than TrainingArguments.

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?

@zach-huggingface, @SunMarc and @qgallouedec

Copy link
Copy Markdown
Member

@SunMarc SunMarc 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 fixing this ! Can you add a test to check the HfArgumentParser so that this doesn't happen ? Also, let's replace union[str, dict] to union[dict, str] in the codebase

@Tavish9
Copy link
Copy Markdown
Contributor Author

Tavish9 commented Jul 17, 2025

Hi, I want to add tests, but as I said, Importing order matters, I cannot test by importing every file in the repo.

Commonly used files: Trainer, PreTrainedModel, AutoModel etc... (too many)

What's your opinion here for the test?

@SunMarc
Copy link
Copy Markdown
Member

SunMarc commented Jul 17, 2025

Let's just restrict ourselves to the most used files like Trainer, AutoModel in the first place and we can potentially add more in the future.

@Tavish9
Copy link
Copy Markdown
Contributor Author

Tavish9 commented Jul 17, 2025

Already done.

I fix the order in the test and we don't need to test importing every module.

Before:

image

After:

image

Copy link
Copy Markdown
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Thanks !

@Tavish9
Copy link
Copy Markdown
Contributor Author

Tavish9 commented Jul 17, 2025

Wait, I found the tests are skipped

SKIPPED [1] tests/utils/test_hf_argparser.py:487: test requires deepspeed

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

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.

@Tavish9
Copy link
Copy Markdown
Contributor Author

Tavish9 commented Jul 17, 2025

Replace deepspeed with accelerator to not be skipped :)

@SunMarc SunMarc enabled auto-merge (squash) July 17, 2025 14:34
@github-actions
Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: modernbert

@SunMarc SunMarc merged commit 73869f2 into huggingface:main Jul 17, 2025
25 checks passed
@Tavish9 Tavish9 deleted the typing branch July 18, 2025 00:48
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Jul 22, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
zaristei pushed a commit to zaristei/transformers that referenced this pull request Sep 9, 2025
* fix type order

* change all Union[str, dict] to Union[dict, str]

* add hf_parser test && fix test order

* add deepspeed dependency

* replace deepspeed with accelerator
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.

HfArgumentParser cannot parse str for local path

3 participants