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

[Fix]: Send model output to cpu before numpy cast in token_clf_pipeline #13819

Closed
wants to merge 2 commits into from
Closed

[Fix]: Send model output to cpu before numpy cast in token_clf_pipeline #13819

wants to merge 2 commits into from

Conversation

ierezell
Copy link
Contributor

What does this PR do?

Fixes #13816

Before submitting

Who can review?

If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of who to tag.
Please tag fewer than 3 people.

Library:

@ierezell
Copy link
Contributor Author

ierezell commented Oct 1, 2021

@Narsil Do you want me to also add a test (all currents tests are passing) ?

in tests/test_pipelines_token_classification.py like :

@require_torch_gpu
@slow
def test_correct_devices(self):
    sentence = "This dummy sentence checks if all the variables can be loaded on gpu and bring back to cpu"
    ner = TokenClassificationPipeline(model="distilbert-base-cased", device=0)    

Have a great day

@Narsil
Copy link
Contributor

Narsil commented Oct 4, 2021

Hi @ierezell .

Unfortunately this fix is not desirable, as we're trying to have all "CPU/GPU" logic be contained inside the Pipeline class (in src/transformers/pipelines/base.py).

I added another proposed fix (the test is still not ran as part of the unit tests (it's still slow) pinging @LysandreJik for advice on if/how we can make GPU tests part of the unit tests.

(I also added you as co-author as you figured out the issue and a solid fix)

@ierezell
Copy link
Contributor Author

ierezell commented Oct 4, 2021

HI @Narsil, indeed it seems better to handle all the logic in the same place. I should've checked before.

I looked at your fix and it's perfect for me.

For the test, unfortunately, even if it's only a really small computation, the test needs GPU... Is it this bad to have it only as CI/CD test and not as a unit test?

Thanks for keeping me as coauthor.

Have a great day!

@Narsil
Copy link
Contributor

Narsil commented Oct 4, 2021

Making ALL pipeline tests GPU as unit regular tests is something that was already raised internally.

Currently pipeline tests are relatively fast, so it really is doable. Slow tests are run on every release + once a day if I am not mistaken so it's already something.

@LysandreJik
Copy link
Member

Closing as resolved by #13856. Let me know if you'd like to reopen this.

@LysandreJik LysandreJik closed this Oct 6, 2021
@ierezell ierezell deleted the ierezell_fix_token_clf_pipeline branch October 6, 2021 13:52
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.

Device error on TokenClassificationPipeline
3 participants