Skip to content

No more Tuple, List, Dict #38797

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

Merged
merged 20 commits into from
Jun 17, 2025
Merged

No more Tuple, List, Dict #38797

merged 20 commits into from
Jun 17, 2025

Conversation

Rocketknight1
Copy link
Member

@Rocketknight1 Rocketknight1 commented Jun 12, 2025

This PR uses @cyyever's work to update types from Tuple/List/Dict to tuple/list/dict. We also add the UP006 ruff check to require this in future.

@Rocketknight1 Rocketknight1 marked this pull request as ready for review June 12, 2025 16:13
@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.

@cyyever
Copy link
Contributor

cyyever commented Jun 13, 2025

The test failed with long output, that's also why I split the PR. I suggest splitting this into smaller ones.

@Rocketknight1
Copy link
Member Author

cc @ydshieh this is actually useful, though - we have a PR that always triggers the CI bug!

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 13, 2025

Too long with no output (exceeded 10m0s): context deadline exceeded

this is not necessary due to long output, this happens from time to time, even with git log -n 1 simple command.

we have a PR that always triggers the CI bug!

Sorry, I d don't understand this command.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you, I just randomly check 2 or 3 files and LGTM.

Please add @cyyever as co-author when click the merge button 🙏 Thank @cyyever to to make the library in a better shape!

The review page is somehow hanging due to large number of files.

The Too long with no output is known CircleCI issue, you can re-run the workflow.

If it persists, ping me and let's see what we could do

For the conflict, I guess it's easier to just reset the branch, fast-forward merge the latest main, apply the same changes and merge immediately once CI is green.

@Rocketknight1
Copy link
Member Author

Sorry, I d don't understand this command.

Hi @ydshieh, sorry, I should have been clearer. What I meant was that tests always fail on this PR with the Too long with no output (exceeded 10m0s): context deadline exceeded bug. That means we can use it for testing - changing a lot of files triggers the problem somehow!

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 13, 2025

Ok got it. But there is also some cases where this happens with just few outputs. But for those, rerins usually works

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 16, 2025

this seems persist 😭 . @Rocketknight1 I can take a look to see if there is some hacky workaround, but if you are up to split the PRs, go ahead too.

@Rocketknight1
Copy link
Member Author

I don't think this is urgent! I can wait, but if you don't think it's worth fixing in the CI let me know and I'll make split PRs instead

@cyyever
Copy link
Contributor

cyyever commented Jun 17, 2025

Is it due to short timeout values? The check took more than 10 minutes on my host.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 17, 2025

It's not something from our side but CircleCI. I can probably change the configuration there but I prefer not to.

Ah, I might have some idea, let me try.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 17, 2025

I was able to make fetch test job work. The change I made for that should be revert before merge.

(I will try to see if I can make it available in a better way later)

The 2 failing tests seems valid, so @Rocketknight1 could you take a look for them? When you want to commit the changes, use something like

git commit -m "[test all]"

Before merge, ping me for the revert (of some changes I made)

@Rocketknight1
Copy link
Member Author

@ydshieh nice, thank you! I'll get the CI to pass here and then ping you again.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 17, 2025

Just in case you need it, the change I made is to change utils/tests_fetcher.py

from

    doctest_list = get_doctest_files()

    print(f"\n### DOCTEST TO RUN ###\n{_print_list(doctest_list)}")

to

    # doctest_list = get_doctest_files()
    doctest_list = []

    print(f"\n### DOCTEST TO RUN ###\n{_print_list(doctest_list)}")

(and when push changes, use the commit message I mentioned)

@Rocketknight1
Copy link
Member Author

cc @ydshieh tests are passing! Should be ready to revert + merge now

@Rocketknight1 Rocketknight1 merged commit 508a704 into main Jun 17, 2025
21 checks passed
@Rocketknight1 Rocketknight1 deleted the py39_type_hints_2 branch June 17, 2025 18:37
@Rocketknight1 Rocketknight1 mentioned this pull request Jun 17, 2025
@ydshieh
Copy link
Collaborator

ydshieh commented Jun 18, 2025

Thanks @Rocketknight1

Thank you @cyyever again for the initiative, and apologize that we forgot to add you as a co-author in the merge commit 🙏 .

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.

4 participants