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

Fixing GPU for token-classification in a better way. #13856

Merged

Conversation

Narsil
Copy link
Contributor

@Narsil Narsil commented Oct 4, 2021

What does this PR do?

We probably need a way to ensure this test is ran !
Superseeds #13819 (different fix for same issue)

Fixes # (issue)

Before submitting

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

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@LysandreJik

Co-authored-by:  Pierre Snell <pierre.snell@botpress.com>
@@ -216,7 +217,7 @@ def _forward(self, model_inputs):
}

def postprocess(self, model_outputs, aggregation_strategy=AggregationStrategy.NONE):
outputs = model_outputs["outputs"]
outputs = model_outputs["outputs"].numpy()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not as familiar as you with the pipeline but will the model_outputs["outputs"] can still be on GPU ?
Maybe all preprocessing is firstly casted to CPU but I would rather double check.

Else should we use model_outputs["outputs"].cpu().numpy() instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ierezell,

Just like this file does not ever mention .to(device) it shouldn't mention .cpu() as it's not the task of the pipeline to encapsulate that logic. Before it was done this way meaning many pipelines wouldn't actually support some features.

The logic is now in the Pipeline.forward method.

Rougly:
preprocess: (generic python objects) -> Tensors
_forward: Tensors -> Tensors
postprocess: Tensors -> Generic python objects

forward encapsulate classic logic for inference (inference mode, GPU tensor moving and other if needed, like batching)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then everything will be at the good place at the good moment so that's perfect! Thanks for the clarification.

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.

Looks good! Thank you, @Narsil @ierezell

@LysandreJik LysandreJik merged commit e7b16f3 into huggingface:master Oct 6, 2021
@Narsil Narsil deleted the fix_gpu_token_classification branch October 6, 2021 09:11
lapisfluvialis pushed a commit to lapisfluvialis/transformers that referenced this pull request Oct 27, 2021
Co-authored-by:  Pierre Snell <pierre.snell@botpress.com>

Co-authored-by: Pierre Snell <pierre.snell@botpress.com>
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 27, 2022
Co-authored-by:  Pierre Snell <pierre.snell@botpress.com>

Co-authored-by: Pierre Snell <pierre.snell@botpress.com>
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.

None yet

3 participants