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

Use Reader's device by default #1208

Merged
merged 4 commits into from
Jun 24, 2021
Merged

Conversation

julian-risch
Copy link
Member

@julian-risch julian-risch commented Jun 18, 2021

Proposed changes:

  • init() method now runs initialize_device_settings(use_cuda=self.use_gpu) to get Reader's device
  • Reader's device is now used as default device of eval(), eval_from_file(), and calibrate_confidence_scores() but can be overwritten when calling these methods
  • doc strings are updated accordingly

Note that train() also calls initialize_device_settings(use_cuda=use_gpu,use_amp=use_amp) as before because train() can be initialized with other a different value for use_gpu than the Reader itself.

closes #1137

@julian-risch julian-risch changed the title WIP:Use Reader's device by default Use Reader's device by default Jun 18, 2021
@julian-risch julian-risch marked this pull request as ready for review June 18, 2021 07:36
@julian-risch julian-risch requested a review from tholor June 18, 2021 07:37
Copy link
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

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

I think there's some overlap with #1182 where another util method get_device was introduced.
Can you please check if we can actually get rid of one of them?

@julian-risch
Copy link
Member Author

I think there's some overlap with #1182 where another util method get_device was introduced.
Can you please check if we can actually get rid of one of them?

Well, the functionality of get_device is not really necessary from my point of you.
Instead of calling get_device, you could always call

self.device, _ = initialize_device_settings(use_cuda=self.use_gpu)

which internally calls:
device = torch.device("cuda" if torch.cuda.is_available() else "cpu")

get_device is the same:

def get_device(use_gpu: bool = True) -> str:
    if use_gpu and torch.cuda.is_available():
        return "cuda"
    return "cpu"

What do you think?

@tholor
Copy link
Member

tholor commented Jun 21, 2021

Yep, seems like they are quite redundant. The only difference: one is returning a torch.device object, the other one a string.
Can you please check if the device object works also in the other locations in Haystack where get_device is currently used?
If yes we can remove get_device() :)

@julian-risch
Copy link
Member Author

By using initialize_device_settings() instead of the get_device() method, the returned device is always a torch.device object.
When this device object is used to transfer models to the GPU (or CPU), the device could also be passed as a parameter of type string:

self.model = RagTokenForGeneration.from_pretrained(model_name_or_path, revision=model_version).to(self.device)

"The torch.device argument in functions can generally be substituted with a string." https://pytorch.org/docs/stable/tensor_attributes.html#torch.torch.device

In rare cases, FARM checks the given device against strings "cpu" or "gpu" but it then extracts the type of the device as follows: gpu=device.type=="cuda"
https://github.com/deepset-ai/FARM/search?q=device.type

Let's use initialize_device_settings() whenever we need to know the device and let's use device.type if we need to check the device against a string.

Copy link
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

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

Thanks for verifying! The changes look good, but can you please verify that the get_device() method is not used anywhere else and if that's the case delete it?

@julian-risch
Copy link
Member Author

Thanks for verifying! The changes look good, but can you please verify that the get_device() method is not used anywhere else and if that's the case delete it?

It's not used. I deleted it now.

@julian-risch julian-risch merged commit 17dcb8c into master Jun 24, 2021
@julian-risch julian-risch deleted the optional_device_param_in_reader branch June 24, 2021 07:22
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.

Optional device parameter in Reader eval method
2 participants