add return_tensor parameter for feature extraction #19257

ajsanjoaquin · 2022-09-30T13:07:30Z

What does this PR do?

Fixes #10016

Before submitting

[ x] 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.
[x ] Did you write any new necessary tests?

Addresses stale issue #10016. Please review @LysandreJik and @Narsil. Thanks.

HuggingFaceDocBuilderDev · 2022-09-30T13:19:00Z

The documentation is not available anymore as the PR was closed or merged.

Narsil

All up for it.

This PR needs to add some kind of test to make sure this argument works

Ideally in tests/pipelines/test_pipelines_feature_extraction.py::run_pipeline_test that way we make sure it works for all pipeline that are supposed to work.

src/transformers/pipelines/feature_extraction.py

ajsanjoaquin · 2022-10-05T17:09:27Z

Feedback has been addressed :)

Narsil · 2022-10-06T07:58:19Z

Not sure what's happening with CircleCI...

src/transformers/pipelines/feature_extraction.py

ajsanjoaquin · 2022-10-07T08:46:41Z

Last suggestion was addressed, but CircleCI Pipeline still seems broken...

Narsil · 2022-10-07T10:25:08Z

@ajsanjoaquin do you have circleCI set up on your account/branch ? I remember people sometimes having issues because of this.

Otherwise, could you try to rebase on main?

Pinging @LysandreJik that might know more.

LysandreJik · 2022-10-10T13:18:03Z

The CircleCI pipeline seems to be working again!

Narsil · 2022-10-10T13:48:59Z

For quality can you try

pip install transformers[dev]
make fixup

?

ajsanjoaquin · 2022-10-10T17:52:29Z

@Narsil when running make fixup, I get the following error. Perhaps it's possible to replicate on your end?

-n was unexpected at this time.
make: *** [Makefile:10: modified_only_fixup] Error 255

Narsil · 2022-10-11T10:09:45Z

-n was unexpected at this time.

What OS/Linux flavor you're running on ? Shell maybe ?
Seems like test -n is not supported in your environment.

ajsanjoaquin · 2022-10-11T12:01:50Z

I'm in a temporary dev environment, so I'm using Windows 😅. I'll use WSL next time. So the formatting error was simply a result of missing newline before importing torch and tf?

Narsil · 2022-10-11T12:25:16Z

I'm in a temporary dev environment, so I'm using Windows sweat_smile. I'll use WSL next time. So the formatting error was simply a result of missing newline before importing torch and tf?

We use black and isort to normalize as much as possible the code, so yes import order is automatically done for you.

ajsanjoaquin · 2022-10-12T08:54:16Z

@Narsil can this PR be merged soon when it passes all tests? I just resolved a conflict made by a different commit on the same file.

Narsil · 2022-10-12T09:32:13Z

@ajsanjoaquin
It seems there is an issue with your CircleCI permissions, the tests won't run.
Could you try refreshing your permissions as shown here ?

ajsanjoaquin · 2022-10-14T20:47:21Z

@Narsil the check_repository_consistency test is failing because of a .md file unrelated to my PR. Do I have to address this in this PR?

check_code_quality is again failing and I can't use make fixup. I request you re-fix the latter.

I hope this can be passed soon before another PR like #19382 modifies the exact code I was working on...

add return_tensor parameter for feature extraction Revert "Merge branch 'feature-extraction-return-tensor' of https://github.com/ajsanjoaquin/transformers into feature-extraction-return-tensor" This reverts commit d559da7, reversing changes made to bbef892.

Co-authored-by: Nicolas Patry <patry.nicolas@protonmail.com>

Narsil

LGTM.

I took the liberty of rebasing and fixing your PR. Is the result OK for you ?

@sgugger for final review.

sgugger

Thanks for adding this! Just one nit on the docstring.

src/transformers/pipelines/feature_extraction.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

sgugger · 2022-10-17T15:55:43Z

This PR was based on an old fork of the repo and as such, the pipeline tests were not run. They do not pass, so I will revert the commit. Could you open a new PR with fixed tests?

This reverts commit 35bd089.

…19680) This reverts commit 35bd089.

Narsil · 2022-10-17T16:24:36Z

I am confused, should we re-open a PR or start from here #19679

(I tried re-opening a PR but it resulted in a no-op...)

* add return_tensors parameter for feature_extraction w/ test add return_tensor parameter for feature extraction Revert "Merge branch 'feature-extraction-return-tensor' of https://github.com/ajsanjoaquin/transformers into feature-extraction-return-tensor" This reverts commit d559da7, reversing changes made to bbef892. * call parameter directly Co-authored-by: Nicolas Patry <patry.nicolas@protonmail.com> * Fixup. * Update src/transformers/pipelines/feature_extraction.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Nicolas Patry <patry.nicolas@protonmail.com> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

…ce#19257)" (huggingface#19680) This reverts commit 35bd089.

LysandreJik requested a review from Narsil October 3, 2022 21:55

Narsil reviewed Oct 5, 2022

View reviewed changes

src/transformers/pipelines/feature_extraction.py Outdated Show resolved Hide resolved

src/transformers/pipelines/feature_extraction.py Outdated Show resolved Hide resolved

ajsanjoaquin force-pushed the feature-extraction-return-tensor branch from b426b78 to a0677bf Compare October 5, 2022 17:07

Narsil reviewed Oct 6, 2022

View reviewed changes

src/transformers/pipelines/feature_extraction.py Outdated Show resolved Hide resolved

ajsanjoaquin force-pushed the feature-extraction-return-tensor branch from fa74148 to 771b975 Compare October 10, 2022 13:39

ajsanjoaquin force-pushed the feature-extraction-return-tensor branch from 0184e44 to 2ab8d05 Compare October 12, 2022 23:35

ajsanjoaquin and others added 3 commits October 17, 2022 11:27

call parameter directly

4af1037

Co-authored-by: Nicolas Patry <patry.nicolas@protonmail.com>

Fixup.

da1c7be

Narsil force-pushed the feature-extraction-return-tensor branch from 2ab8d05 to da1c7be Compare October 17, 2022 09:31

Narsil approved these changes Oct 17, 2022

View reviewed changes

sgugger approved these changes Oct 17, 2022

View reviewed changes

src/transformers/pipelines/feature_extraction.py Outdated Show resolved Hide resolved

Update src/transformers/pipelines/feature_extraction.py

efa30d0

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

sgugger merged commit 35bd089 into huggingface:main Oct 17, 2022

sgugger mentioned this pull request Oct 17, 2022

Fix imports in pipeline tests #19679

Closed

sgugger added a commit that referenced this pull request Oct 17, 2022

Revert "add return_tensor parameter for feature extraction (#19257)"

e88b95f

This reverts commit 35bd089.

sgugger mentioned this pull request Oct 17, 2022

Revert "add return_tensor parameter for feature extraction" #19680

Merged

sgugger added a commit that referenced this pull request Oct 17, 2022

Revert "add return_tensor parameter for feature extraction (#19257)" (#…

f2ecb9e

…19680) This reverts commit 35bd089.

ajsanjoaquin deleted the feature-extraction-return-tensor branch October 18, 2022 15:13

kashif pushed a commit to kashif/transformers that referenced this pull request Oct 21, 2022

Revert "add return_tensor parameter for feature extraction (huggingfa…

7df7c2c

…ce#19257)" (huggingface#19680) This reverts commit 35bd089.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add return_tensor parameter for feature extraction #19257

add return_tensor parameter for feature extraction #19257

ajsanjoaquin commented Sep 30, 2022

HuggingFaceDocBuilderDev commented Sep 30, 2022 •

edited

Loading

Narsil left a comment

ajsanjoaquin commented Oct 5, 2022

Narsil commented Oct 6, 2022

ajsanjoaquin commented Oct 7, 2022

Narsil commented Oct 7, 2022

LysandreJik commented Oct 10, 2022

Narsil commented Oct 10, 2022

ajsanjoaquin commented Oct 10, 2022 •

edited

Loading

Narsil commented Oct 11, 2022

ajsanjoaquin commented Oct 11, 2022

Narsil commented Oct 11, 2022

ajsanjoaquin commented Oct 12, 2022

Narsil commented Oct 12, 2022 •

edited

Loading

ajsanjoaquin commented Oct 14, 2022

Narsil left a comment

sgugger left a comment

sgugger commented Oct 17, 2022

Narsil commented Oct 17, 2022

add return_tensor parameter for feature extraction #19257

add return_tensor parameter for feature extraction #19257

Conversation

ajsanjoaquin commented Sep 30, 2022

What does this PR do?

Before submitting

HuggingFaceDocBuilderDev commented Sep 30, 2022 • edited Loading

Narsil left a comment

Choose a reason for hiding this comment

ajsanjoaquin commented Oct 5, 2022

Narsil commented Oct 6, 2022

ajsanjoaquin commented Oct 7, 2022

Narsil commented Oct 7, 2022

LysandreJik commented Oct 10, 2022

Narsil commented Oct 10, 2022

ajsanjoaquin commented Oct 10, 2022 • edited Loading

Narsil commented Oct 11, 2022

ajsanjoaquin commented Oct 11, 2022

Narsil commented Oct 11, 2022

ajsanjoaquin commented Oct 12, 2022

Narsil commented Oct 12, 2022 • edited Loading

ajsanjoaquin commented Oct 14, 2022

Narsil left a comment

Choose a reason for hiding this comment

sgugger left a comment

Choose a reason for hiding this comment

sgugger commented Oct 17, 2022

Narsil commented Oct 17, 2022

HuggingFaceDocBuilderDev commented Sep 30, 2022 •

edited

Loading

ajsanjoaquin commented Oct 10, 2022 •

edited

Loading

Narsil commented Oct 12, 2022 •

edited

Loading