Add ability to load multiple copies of a model across processes #31052
Conversation
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| @@ -234,6 +235,9 @@ def __init__( | |||
| memory pressure if you load multiple copies. Given a model that | |||
| consumes N memory and a machine with W cores and M memory, you should | |||
| set this to True if N*W > M. | |||
| model_copies: The exact number of models that you would like loaded | |||
| onto your machine. This can be useful if you exactly know your CPU or | |||
There was a problem hiding this comment.
Possible wording suggestion:
This can be useful if you exactly know your CPU or GPU capacity and want to maximize resource utilization.
If set, large_model becomes a no-op.
Maybe this should be a ValueError if a user specifies both?
There was a problem hiding this comment.
Possible wording suggestion:
Updated
Maybe this should be a ValueError if a user specifies both?
I don't think it should be a ValueError - if you change it to True and you set this param, that is kinda reasonable and a no-op makes sense IMO since we're still honoring your choice.
There was a problem hiding this comment.
ah my concern was not an incorrect configuration but cognitive burden for users: would they be thinking if they should set only one param, or both in their use case, while in the end it doesn't matter. but now it also seems that large_model becomes redundant as it is equivalent to passing model_copies = 1, right?
Possibly except the fact that using model_copies is currently disallowed with KeyedMH, and large_model might still allow that.
There was a problem hiding this comment.
but now it also seems that large_model becomes redundant as it is equivalent to passing model_copies = 1, right?
That's right, though I think long term I would like for us to do smart things here (e.g. large_model becomes "pack as many models as you can fit). There's some conversation on this general idea in the design doc
| @@ -234,6 +235,9 @@ def __init__( | |||
| memory pressure if you load multiple copies. Given a model that | |||
| consumes N memory and a machine with W cores and M memory, you should | |||
| set this to True if N*W > M. | |||
| model_copies: The exact number of models that you would like loaded | |||
| onto your machine. This can be useful if you exactly know your CPU or | |||
There was a problem hiding this comment.
ah my concern was not an incorrect configuration but cognitive burden for users: would they be thinking if they should set only one param, or both in their use case, while in the end it doesn't matter. but now it also seems that large_model becomes redundant as it is equivalent to passing model_copies = 1, right?
Possibly except the fact that using model_copies is currently disallowed with KeyedMH, and large_model might still allow that.
| @@ -952,6 +977,12 @@ def get_preprocess_fns(self) -> Iterable[Callable[[Any], Any]]: | |||
| def should_skip_batching(self) -> bool: | |||
| return True | |||
|
|
|||
| def share_model_across_processes(self) -> bool: | |||
There was a problem hiding this comment.
(cleanup, can be deferred)
we can leverage reflection here and delegate calls to base via __getattr__ like in
Per https://stackoverflow.com/questions/2405590/how-do-i-override-getattr-without-breaking-the-default-behavior, explicitly defined methods should take priority.
There was a problem hiding this comment.
This is a good idea. I think we should do it, but agree deferring is a good idea to keep the PR to a single purpose
* Add ability to load multiple copies of a model across processes * push changes I had locally not remotely * Lint * naming + lint * Changes from feedback
…) (#31104) * Add ability to load multiple copies of a model across processes * push changes I had locally not remotely * Lint * naming + lint * Changes from feedback
Sometimes loading one model per process isn't feasible, but you still want multiple models loaded (e.g. if you have a GPU that can hold 3 copies of the model). This gives users the ability to express this.
Design doc - https://docs.google.com/document/d/1FmKrBHkb8YTYz_Dcec7JlTqXwy382ar8Gxicr_s13c0/edit
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.