Improve BatchFeature: stack list and lists of torch tensors #42750
Conversation
yonigozlan
force-pushed
the
improve-batch-feature
branch
from
5be88b5 to
3a4cecd
Compare
|
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. |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Great PR! Left a few questions to make sure I got it right
| return BatchFeature( | ||
| data={"pixel_values": processed_images, "patch_offsets": patch_offsets}, | ||
| tensor_type=return_tensors, | ||
| skip_tensor_conversion=["patch_offsets"], |
There was a problem hiding this comment.
For my understanding, do we skip the conversion because offsets aren't padded?
There was a problem hiding this comment.
Yep! We used to create the batch then add the non-converted path_offsets, but I fell it's cleaner to be explicit this way
| BatchFeature class for Fuyu image processor and processor. | ||
| The outputs dictionary from the processors contains a mix of tensors and lists of tensors. | ||
| """ |
There was a problem hiding this comment.
looks like Fuyu has its own BatchFeature because we couldn't skip conversion or convert nested lists? 🤔 Can you inspect and maybe remove it if possible, now that the base class supports skipping?
There was a problem hiding this comment.
Yes I can do that in another PR! I didn't look into too much details what the Fuyu batch feature does, but it would be great to get rid of it
|
|
||
| # stack list of tensors if tensor_type is PyTorch (# torch.tensor() does not support list of tensors) | ||
| if isinstance(value, (list, tuple)) and len(value) > 0 and torch.is_tensor(value[0]): | ||
| return torch.stack(value) | ||
|
|
||
| # convert list of numpy arrays to numpy array (stack) if tensor_type is Numpy |
There was a problem hiding this comment.
i dunno if you saw the PR. Community member noticed that VideoMetadata objects throw and error when return type is 'pt', because they can't be converted to tensors
I think we can add the fix here by checking if value is a list/array/etc and early existing otherwise. We won't be able to convert non-list objects anyway
There was a problem hiding this comment.
Agreed, I was just wondering why we restricted BatchFeature to only be able to contain arrays/tensors structures in the first place, just to make sure we wouldn't break an important assumption by silently allowing other objects in BatchFeature.
Also these changes should be made along with changes to the ".to()" method no?
|
|
||
| mean_value = round(pixel_values.mean().item(), 4) | ||
| self.assertEqual(mean_value, 0.2353) | ||
| self.assertEqual(mean_value, -0.2303) |
There was a problem hiding this comment.
to make sure: was it a typo or did this PR change pixel outputs?
There was a problem hiding this comment.
I'll have to check on a CI runner if that value is correct, but this tests failed on main on my end (using an A10)
| class BatchFeatureTester(unittest.TestCase): | ||
| """Tests for the BatchFeature class and tensor conversion.""" | ||
|
|
There was a problem hiding this comment.
thanks for adding a test 🤩
|
[For maintainers] Suggested jobs to run (before merge) run-slow: beit, bridgetower, cohere2_vision, convnext, deepseek_vl, deepseek_vl_hybrid, depth_pro, dinov3_vit, donut, dpt, efficientloftr, efficientnet, eomt, flava, fuyu, gemma3 |
yonigozlan
force-pushed
the
improve-batch-feature
branch
from
e2ceac2 to
875c36e
Compare
yonigozlan
changed the title
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=42750&sha=e2ceac |
3 participants
I have been wanting to change that for a while, it shouldn't be a breaking change, but align what we support in
BatchFeaturebetween numpy arrays and torch tensors.The issue was that np.array() works on lists of array and even nested list of arrays), but torch.tensor() doesn't, so if we tried to call batch feature, with lists of tensors, we would get an error. I haven't added support for nested lists of tensors as I haven't seen the need anywhere.
Also the errors we were getting were very generic, and not very useful, this should help with that.