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

Check for mismatch in device setup in evaluator #287

Merged
merged 2 commits into from
Sep 9, 2022

Conversation

mathemakitten
Copy link
Contributor

@mathemakitten mathemakitten commented Sep 2, 2022

Per #285, we missed an edge case in the compute function for the evaluator where if you pass a pipeline which was already instantiated without a device then it won't run on device even if a device is later passed to compute.

Alternatives considered were:

  • Change nothing, but warn the user that they've passed a device to compute but their pipeline doesn't have a device attached. Bad because users can mindlessly scroll past it.
  • Override the device used in the pipeline when a device has been detected but not used, and throw a different warning to let them know. Seems error-prone in multi-device setups, and reinstantiating already-instantiated pipes seems very bad.
    So I went with @lhoestq's suggestion to raise an error to force the user to disambiguate.

Tests with:

from datasets import Dataset, load_dataset
from evaluate import evaluator
from transformers import pipeline

data = load_dataset("Maysee/tiny-imagenet", split="valid", use_auth_token=True)

pipe = pipeline(
    task="image-classification",
    model="google/vit-base-patch16-224",
    # device=0  # If this commented it will trigger an error
    )

task_evaluator = evaluator("image-classification")
eval_results = task_evaluator.compute(
    model_or_pipeline=pipe,
    data=data,
    metric="accuracy",
    label_mapping=pipe.model.config.label2id,
    device=0
)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 2, 2022

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

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Looks great! Could you add a few tests to check if the desired behaviour is achieved?

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Cool ! Raising an error is definitely ok in ambiguous situations

Comment on lines 282 to 285
elif device != model_or_pipeline.device.index:
logger.warning(
f"This pipeline was instantiated on device {model_or_pipeline.device.index} but device={device} was passed to `compute`."
)
Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to raise an error is this case as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Also added some tests :)

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Thank you !

@mathemakitten mathemakitten merged commit eef73da into main Sep 9, 2022
@mathemakitten mathemakitten deleted the hn-error-if-unclear-device-args branch September 9, 2022 15:34
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

4 participants