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

Refactor StreamInferHandler for better readability #6257

Closed
wants to merge 6 commits into from

Conversation

rmccorm4
Copy link
Contributor

@rmccorm4 rmccorm4 commented Sep 1, 2023

This PR does two things:

  1. Refactor the handling of every step in StreamInferHandler::Process from one large ~400 line function into helper functions for each state.
  2. Replace the "double negative" logic of if not not_finished into just if finished in Common/InferHandler/StreamInferHandler Process() functions. I found the double negative logic confusing that the function did:
bool Process(...) {
  ...
  finished = true; 
  // (not finished)
  return !finished;
}

and then the caller checks the negation of that:

// if (not (not finished) )
if (  !Process(...) )

So instead if's more direct to be:

bool Process(...) {
  finished = true;
  return finished;
}

if ( Process(...) {
  ...
}

src/grpc/stream_infer_handler.h Outdated Show resolved Hide resolved
bool finished = false;

if (state->step_ == Steps::START) {
finished = RequestStartStep(state, rpc_ok);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pull the step transition from within these helpers to this Process function? It would help in giving a complete picture of the state machine being implemented here.
If not, then may be add a comment describing to which state the state can transition to in these helper functions.

Copy link
Contributor Author

@rmccorm4 rmccorm4 Sep 6, 2023

Choose a reason for hiding this comment

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

This would be nice, but there's a lot of branching in each function where if (this) state = state1; else state = state2; It would be non-trivial effort to extract this out I think.

I added some descriptive comments for now.

Comment on lines +467 to +468
// Write the next response if it is ready...
state->context_->WriteResponseIfReady(nullptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tanmayv25 I don't understand this part. Do you know why we try to Write the next response (not the current' state's response) here in the WRITTEN state for a non-decoupled request?

For the non-decoupled case, isn't every response's write handled when calling the response callback or error handling for a given request?

Comment on lines 362 to 364
state->step_ = Steps::FINISH;
// Finished with the request.
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also @tanmayv25 , I don't really understand the point of the COMPLETE state/step. Why don't we directly transition to FINISH state anywhere we would transition to COMPLETE instead? Seems like an unnecessary intermediate step.

…, break out decoupled handling in Process() directly
@rmccorm4
Copy link
Contributor Author

rmccorm4 commented Oct 5, 2023

Closing due to lots of changes around request cancellation. May come back to this later.

@rmccorm4 rmccorm4 closed this Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants