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

Add datasets.Dataset to Trainer's train_dataset and eval_dataset type hints #30077

Conversation

ringohoffman
Copy link
Contributor

What does this PR do?

Adds datasets.Dataset to Trainer's type hints for train_dataset and eval_dataset.

It is supported and mentioned by name in the docstrings for these parameters, but is not hinted as an acceptable type. Currently, if you try to call Trainer with a datasets.Dataset for one of these parameters, type hinters like pyright raise an error.

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.

@amyeroberts
@muellerzr
@pacman100

Copy link
Collaborator

@amyeroberts amyeroberts 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 adding!

I'm going to accept, as the type hint is useful to users. It should be noted that although we have type hints in the library, there isn't full coverage and we don't guaranteed no errors for type checking tools such as pyright or mypy.

src/transformers/trainer.py Show resolved Hide resolved
@amyeroberts
Copy link
Collaborator

Rebasing onto main should resolve the unrelated test errors

@ringohoffman ringohoffman requested a review from amyeroberts April 8, 2024 20:57
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks!

@amyeroberts amyeroberts merged commit af4c026 into huggingface:main Apr 9, 2024
20 checks passed
@ringohoffman ringohoffman deleted the add-datasets-dataset-to-trainer-dataset-type-hints branch April 9, 2024 17:01
ArthurZucker pushed a commit that referenced this pull request Apr 22, 2024
… hints (#30077)

* Add datasets.Dataset to Trainer's train_dataset and eval_dataset type hints

* Add is_datasets_available check for importing datasets under TYPE_CHECKING guard

https://github.com/huggingface/transformers/pull/30077/files#r1555939352
itazap pushed a commit that referenced this pull request May 14, 2024
… hints (#30077)

* Add datasets.Dataset to Trainer's train_dataset and eval_dataset type hints

* Add is_datasets_available check for importing datasets under TYPE_CHECKING guard

https://github.com/huggingface/transformers/pull/30077/files#r1555939352
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.

2 participants