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

feat!: Framework-agnostic device management #6748

Merged
merged 7 commits into from
Jan 17, 2024

Conversation

shadeMe
Copy link
Collaborator

@shadeMe shadeMe commented Jan 15, 2024

Related Issues

Proposed Changes:

This PR introduces the concept of framework-agnostic device representations. The main impetus behind this change is to move away from stringified representations of devices that are not portable between different frameworks. It also enables support for multi-device inference in a generic manner.

device.py contains the following new classes:

  • DeviceType - An enum representing the types of devices we support.
  • Device - A tuple of a DeviceType and an integer identifier that represents a single device.
  • DeviceMap - An arbitrary mapping of model parameters to single devices, similar to HF's device_map in their accelerate library.
  • ComponentDevice - Essentially a tagged union of Device and DeviceMap. This class consumed by downstream components.

Going forward, components can expose a single, optional device parameter in their constructor (Optional[ComponentDevice]). The component can then decide what to do with it:

  • If None, the component can either do nothing or automatically pick the best device to load the model. In the case of the latter, it is done by simply passing the optional parameter to ComponentDevice.resolve_device(device).
  • If not None, the component can still call ComponentDevice.resolve_device(device). In this case, the function will immediately return the passed device without any modification.
  • After device resolution, the ComponentDevice instance can optionally be persisted in the component itself. Since the class itself is not trivially serializable to JSON, the Component.to_dict and Component.from_dict functions must explicitly call ComponentDevice.to_dict/from_dict during serde.
  • Finally, the ComponentDevice can be converted to the component's backend representation by calling its to_xxx methods.

The following components have been updated to support the above workflow: NamedEntityExtractor, HuggingFaceLocalGenerator, TransformersSimilarityRanker, ExtractiveReader.

How did you test it?

Unit and e2e tests.

Notes for the reviewer

  • I'm not happy with the ComponentDevice name. Suggestions are welcome.

Checklist

@shadeMe shadeMe added type:feature New feature or request breaking change 2.x Related to Haystack v2.0 labels Jan 15, 2024
@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Jan 15, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jan 16, 2024

Pull Request Test Coverage Report for Build 7545294479

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 88.01%

Totals Coverage Status
Change from base Build 7543114073: 0.1%
Covered Lines: 4529
Relevant Lines: 5146

💛 - Coveralls

@shadeMe shadeMe marked this pull request as ready for review January 16, 2024 12:02
@shadeMe shadeMe requested review from a team as code owners January 16, 2024 12:02
@shadeMe shadeMe requested review from dfokina, ZanSara and sjrl and removed request for a team January 16, 2024 12:02
@sjrl
Copy link
Contributor

sjrl commented Jan 16, 2024

Hey @shadeMe this is looking really good! I'm still looking through the PR, but I thought I would add a few questions that have already popped into mind:

  • With the current setup if users want to specify their own device are we requiring them to have to pass a ComponentDevice which means if they want to pass "cuda:0" they would have to use ComponentDevice.from_str("cuda:0")? Do you think it would make sense for convenience to allow users to pass in a string representation as well instead of needing it to be a ComponentDevice? Or are there some problems with this approach?
  • Could you provide an example of how a user could create a DeviceMap version of ComponentDevice? I'm curious to figure this out for the PR feat: Add support for device_map #6679

@shadeMe
Copy link
Collaborator Author

shadeMe commented Jan 16, 2024

* With the current setup if users want to specify their own device are we requiring them to have to pass a `ComponentDevice` which means if they want to pass `"cuda:0"` they would have to use `ComponentDevice.from_str("cuda:0")`? Do you think it would make sense for convenience to allow users to pass in a string representation as well instead of needing it to be a `ComponentDevice`? Or are there some problems with this approach?

There's no inherent limitation that prevents us from converting that parameter into a Union[str, ComponentDevice] and have ComponentDevice.resolve_device call from_str internally. That being said, I think there's an argument to be made for keeping the API boundaries as explicit as possible and avoid untagged Unions.

* Could you provide an example of how a user could create a `DeviceMap` version of `ComponentDevice`? I'm curious to figure this out for the PR [feat: Add support for `device_map` #6679](https://github.com/deepset-ai/haystack/pull/6679)
device_map = ComponentDevice.from_multiple(DeviceMap({
	"classifier": Device.gpu(1),
	"layer_1": Device.cpu(),
	"lm_head": Device.disk()
}))

device_map = ComponentDevice.from_multiple(DeviceMap.from_hf({
	"classifier": 1,
	"layer_1": "cpu",
	"lm_head": "disk"
}))

It's not super ergonomic and there's admittedly a lot of ceremony, but that's the tradeoff for being more explicit/generic.

Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Left a few comments, in general looks good!

haystack/utils/device.py Outdated Show resolved Hide resolved
haystack/utils/device.py Show resolved Hide resolved
haystack/utils/device.py Outdated Show resolved Hide resolved
haystack/utils/device.py Outdated Show resolved Hide resolved
test/components/extractors/test_named_entity_extractor.py Outdated Show resolved Hide resolved
test/utils/test_device.py Show resolved Hide resolved
@sjrl
Copy link
Contributor

sjrl commented Jan 16, 2024

Oh one other thing that would be very useful is that when using device_map with multiple devices it would be great if we could have an easy way to access the first device in the list. I'm not entirely sure if that is setup out of the box here. Ofc we can access this by doing a lookup on the underlying DeviceMap and converting the first Device in that dict.

I can see how this looks and feels when finishing up the device_map PR where the first device in the list is needed to know where to pass the output of the tokenizer to get it into the model.

@shadeMe
Copy link
Collaborator Author

shadeMe commented Jan 16, 2024

Oh one other thing that would be very useful is that when using device_map with multiple devices it would be great if we could have an easy way to access the first device in the list. I'm not entirely sure if that is setup out of the box here. Ofc we can access this by doing a lookup on the underlying DeviceMap and converting the first Device in that dict.

Ah, good point. I can add a property for that.

Copy link
Contributor

@sjrl sjrl left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@masci masci requested a review from ZanSara January 17, 2024 09:19
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

LGTM 😊

@shadeMe shadeMe merged commit 7376838 into deepset-ai:main Jan 17, 2024
23 checks passed
@shadeMe shadeMe deleted the feat/device-management branch January 17, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to Haystack v2.0 breaking change topic:tests type:documentation Improvements on the docs type:feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GPU/devices management in Haystack 2.x
4 participants