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
| 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?
There was a problem hiding this comment.
I think so, and we never had a variety of model inputs in the past. Usually whatever is output from processor goes directly in forward, so 99% chance it's an array-like object
IMO we can break the assumption now
There was a problem hiding this comment.
Ok sgtm, I might do that in another PR though, as this would be quite a big change, and it might be lost in the 63+ files modified here just due to allowing stacking tensors
|
|
||
| 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)
|
[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 |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Oke, thanks, let's not forget about Fuyu and video metadata in subsequent PRs
…ace#42750) * stack lists of tensors in BatchFeature, improve error messages, add tests * remove unnecessary stack in fast image processors and video processors * make style * fix tests
* remove attributes and add all missing sub processors to their auto classes * remove all mentions of .attributes * cleanup * fix processor tests * fix modular * remove last attributes * fixup * fixes after merge * fix wrong tokenizer in auto florence2 * fix missing audio_processor + nits * Override __init__ in NewProcessor and change hf-internal-testing-repo (temporarily) * fix auto tokenizer test * add init to markup_lm * update CustomProcessor in custom_processing * remove print * nit * refactor processor tests first part * refactor part 2 * fix test modeling owlv2 * fix test_processing_layoutxlm * Fix owlv2, wav2vec2, markuplm, voxtral issues * part3 * refactor all processor with mixin * add support for loading and saving multiple tokenizer natively * remove exclude_attributes from save_pretrained * get processor from pretrained instead of components in tests * skip tests in colqwen2, pixtral * modifs after review * fix style and copies * Fix after review * add test_processor_from_pretrained_vs_from_components, fix failing tests * fix overflowing_tokens tests * add config for layoutxlm * fix ci * use modular * fic docstring * Fix most tests * Standardize mgp_str tests * fix oneformer processing tests + fix copies * fix after review * fix missing fet_images in fast image processors * fix 01 - to check * fix 02 - to check * fix 03 - to check * fix 03 - to check * fix 03 - to check * fix 04 - to check * fix 05 - to check * fix 06 - sytle * fix 07 - revert * Fix some errors * Improve BatchFeature: stack list and lists of torch tensors (#42750) * stack lists of tensors in BatchFeature, improve error messages, add tests * remove unnecessary stack in fast image processors and video processors * make style * fix tests * fix remaining tests * fix copies * Fix Lfm2_vl im proc test * nit after review --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
…ace#42750) * stack lists of tensors in BatchFeature, improve error messages, add tests * remove unnecessary stack in fast image processors and video processors * make style * fix tests
* remove attributes and add all missing sub processors to their auto classes * remove all mentions of .attributes * cleanup * fix processor tests * fix modular * remove last attributes * fixup * fixes after merge * fix wrong tokenizer in auto florence2 * fix missing audio_processor + nits * Override __init__ in NewProcessor and change hf-internal-testing-repo (temporarily) * fix auto tokenizer test * add init to markup_lm * update CustomProcessor in custom_processing * remove print * nit * refactor processor tests first part * refactor part 2 * fix test modeling owlv2 * fix test_processing_layoutxlm * Fix owlv2, wav2vec2, markuplm, voxtral issues * part3 * refactor all processor with mixin * add support for loading and saving multiple tokenizer natively * remove exclude_attributes from save_pretrained * get processor from pretrained instead of components in tests * skip tests in colqwen2, pixtral * modifs after review * fix style and copies * Fix after review * add test_processor_from_pretrained_vs_from_components, fix failing tests * fix overflowing_tokens tests * add config for layoutxlm * fix ci * use modular * fic docstring * Fix most tests * Standardize mgp_str tests * fix oneformer processing tests + fix copies * fix after review * fix missing fet_images in fast image processors * fix 01 - to check * fix 02 - to check * fix 03 - to check * fix 03 - to check * fix 03 - to check * fix 04 - to check * fix 05 - to check * fix 06 - sytle * fix 07 - revert * Fix some errors * Improve BatchFeature: stack list and lists of torch tensors (huggingface#42750) * stack lists of tensors in BatchFeature, improve error messages, add tests * remove unnecessary stack in fast image processors and video processors * make style * fix tests * fix remaining tests * fix copies * Fix Lfm2_vl im proc test * nit after review --------- Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
4 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.
Needed for #41388