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

[AIR] Execute GPU inference in a separate stage in BatchPredictor #26616

Merged
merged 29 commits into from
Jul 20, 2022

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Jul 15, 2022

Why are these changes needed?

This PR is stacked on #26600

@ericl ericl added the do-not-merge Do not merge this PR! label Jul 15, 2022
@@ -231,6 +253,9 @@ def predict_pipelined(
max_scoring_workers: If set, specify the maximum number of scoring actors.
num_cpus_per_worker: Number of CPUs to allocate per scoring worker.
num_gpus_per_worker: Number of GPUs to allocate per scoring worker.
separate_gpu_stage: If using GPUs, specifies whether to execute GPU
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case when we would not want this enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the preprocessor is very lightweight, then enabling this could hurt more than it helps, due to excess disk spilling (in non-pipelined mode).

@ericl ericl changed the title [WIP] [AIR] Execute GPU inference in a separate stage in BatchPredictor [AIR] Execute GPU inference in a separate stage in BatchPredictor Jul 19, 2022
Signed-off-by: Eric Liang <[email protected]>
Signed-off-by: Eric Liang <[email protected]>
@ericl ericl removed the do-not-merge Do not merge this PR! label Jul 19, 2022
if num_gpus_per_worker is None:
num_gpus_per_worker = 0
if num_cpus_per_worker is None:
if num_gpus_per_worker > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be doing this across all of our library code 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's probably a good idea.

if separate_gpu_stage and num_gpus_per_worker > 0:
preprocessor = self.get_preprocessor()
if preprocessor:
override_prep = BatchMapper(lambda x: x)
Copy link
Contributor

Choose a reason for hiding this comment

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

doesnt seem like override_prep is being used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's weird LINT didn't catch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's actually a local variable from the previous PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there might be a set_preprocessor line missing to disable the preprocessor for the predictor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually correct--- I modified the unit test. If you remove this line, the test will fail. Also added a clarifying comment.

Signed-off-by: Eric Liang <[email protected]>
Signed-off-by: Eric Liang <[email protected]>
Signed-off-by: Eric Liang <[email protected]>
ericl and others added 2 commits July 19, 2022 17:26
@ericl ericl merged commit 0f6beca into ray-project:master Jul 20, 2022
Rohan138 pushed a commit to Rohan138/ray that referenced this pull request Jul 28, 2022
Stefan-1313 pushed a commit to Stefan-1313/ray_mod that referenced this pull request Aug 18, 2022
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.

3 participants