Skip to content
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

docs: fix return type annotation of get_default_model_revision #35982

Conversation

MarcoGorelli
Copy link
Contributor

@MarcoGorelli MarcoGorelli commented Jan 31, 2025

What does this PR do?

Fixes #35981

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.

@MarcoGorelli MarcoGorelli marked this pull request as ready for review January 31, 2025 10:59
@Rocketknight1 Rocketknight1 force-pushed the get-default-model-and-revision-return-type branch from 5dd1647 to 013bec1 Compare January 31, 2025 16:11
@Rocketknight1
Copy link
Member

Yes, this looks good! Rebased the PR and enabled doctests

@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.

@Rocketknight1
Copy link
Member

Hi @MarcoGorelli, the rebase fixed most issues but the failing TF test looks related - maybe the assumption that the return value is always Tuple[str, str] is not true in some TF cases?

@MarcoGorelli
Copy link
Contributor Author

thanks @Rocketknight1 for your review!

the traceback shows

>       with open(vocab_file, encoding="utf-8") as vocab_handle:
E       TypeError: expected str, bytes or os.PathLike object, not NoneType

and I wouldn't have expected the type annotation to influence that. Taking a look anyway

Any chance you could help me with running the test please? I tried running

RUN_PIPELINE_TESTS=1 pytest tests/models/whisper/test_modeling_tf_whisper.py::TFWhisperModelTest::test_pipeline_feature_extraction

but it still gets skipped

@Rocketknight1
Copy link
Member

Hi @MarcoGorelli, tests are passing now! Were the failures earlier just a coincidence that were solved with a rebase?

@MarcoGorelli
Copy link
Contributor Author

Awesome! I think so

@Rocketknight1
Copy link
Member

Rocketknight1 commented Feb 10, 2025

LGTM, in that case! cc @ArthurZucker @Cyrilvallez for core maintainer review

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks!

@ArthurZucker ArthurZucker merged commit 3c912c9 into huggingface:main Feb 13, 2025
25 checks passed
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.

Docs: return type of get_default_model_and_revision might be incorrectly documented?
4 participants