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

BatchFeature.to() supports non-tensor keys #33918

Merged
merged 9 commits into from
Oct 8, 2024
Merged

Conversation

Rocketknight1
Copy link
Member

@Rocketknight1 Rocketknight1 commented Oct 3, 2024

This PR fixes a bug in the preprocessing for several pipelines. The pipelines were calling .to() on a BatchFeature which had a string feature, which caused an error. This update was also requested internally in Slack!

cc @ydshieh

@ydshieh
Copy link
Collaborator

ydshieh commented Oct 3, 2024

Thank you @Rocketknight1 .

2 questions:

  • Is this the only one to change? I see 3 failed tests (in torch pipeline CI reports).
  • Do you think it makes sense to have this implemented directly in BatchFeature instead of outside it?

@Rocketknight1
Copy link
Member Author

@ydshieh good spot, I fixed it for all three! We could consider rewriting BatchFeature.to() like you suggested though, let me ask people

@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 Rocketknight1 force-pushed the fix_oneformer_pipeline branch from a9ac41e to 4d796f7 Compare October 4, 2024 14:47
@Rocketknight1 Rocketknight1 changed the title Fix issue in oneformer pipeline preprocessing BatchFeature.to() supports non-tensor keys Oct 4, 2024
@Rocketknight1
Copy link
Member Author

cc @ydshieh I changed the PR to update BatchFeature.to() instead, which means the individual pipeline fixes aren't needed anymore

@Rocketknight1
Copy link
Member Author

cc @ArthurZucker for core maintainer review! This PR is simple: It patches BatchFeature.to() to check that its keys are tensors before it calls .to() on them. This means that BatchFeature.to() works even with string keys now, which used to throw an error.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Thanks @Rocketknight1!

@LysandreJik
Copy link
Member

Are there other areas that should be updated? Arthur tells me about Pixtral as an example

@Rocketknight1
Copy link
Member Author

@LysandreJik I checked the codebase for methods overriding to(). Mostly this was in modeling code to create objects like NestedTensor - these are safe because their contents should always be torch.Tensor.

The exceptions were:

  • Pixtral
  • BatchEncoding (the tokenizer output class)

In both cases, I updated their methods to make sure they only call to() on torch.Tensor!

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

perfect, thanks!

@Rocketknight1 Rocketknight1 merged commit fb360a6 into main Oct 8, 2024
23 checks passed
@Rocketknight1 Rocketknight1 deleted the fix_oneformer_pipeline branch October 8, 2024 12:43
NielsRogge pushed a commit to NielsRogge/transformers that referenced this pull request Oct 21, 2024
* Fix issue in oneformer preprocessing

* [run slow] oneformer

* [run_slow] oneformer

* Make the same fixes in DQA and object detection pipelines

* Fix BatchFeature.to() instead

* Revert pipeline-specific changes

* Add the same check in Pixtral's methods

* Add the same check in BatchEncoding

* make sure torch is imported
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* Fix issue in oneformer preprocessing

* [run slow] oneformer

* [run_slow] oneformer

* Make the same fixes in DQA and object detection pipelines

* Fix BatchFeature.to() instead

* Revert pipeline-specific changes

* Add the same check in Pixtral's methods

* Add the same check in BatchEncoding

* make sure torch is imported
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
* Fix issue in oneformer preprocessing

* [run slow] oneformer

* [run_slow] oneformer

* Make the same fixes in DQA and object detection pipelines

* Fix BatchFeature.to() instead

* Revert pipeline-specific changes

* Add the same check in Pixtral's methods

* Add the same check in BatchEncoding

* make sure torch is imported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants