[Service] Refactors service to align with EX545081

[allenwang28]

Specifically, this PR:

• Introduces replica, which:
  • handles process lifecycle, async request queueing, fault recovery
  • modularizes it a bit more than the prior implementation
• Since replica pulls in functionality of RecoverableProcMesh, we remove it altogether
• Refactors service to use Replica
• Modifies test_service to be correct

Next PRs:

• add endpoints to service, to retain the ability to call() and choose()
• place replicas on their own procs
• place services on their own procs
• introduce sharded/distributed services? Or multiple services for envs

[Jack-Khuu]

Still reviewing, but love it so far

Thanks for doing the refactor

[Jack-Khuu]

Logic looks legit to me. A couple of questions, but we should be ready to go

[joecummings]

nit: couldn't we just set a larger timeout and/or parameterize it? Seems cleaner than a try except for timeout that allows to continue polling.

[allenwang28]

The timeout is parameterized already and we can't set that to infinity because then we won't be able to stop the replica

We shouldn't remove this except but we can set the variable to a high number, it just affects how long it takes to shutdown

[joecummings]

Why is this needed?

[allenwang28]

it's to avoid a busy-wait (like put back in the queue and check again) which would take 100% of the cycles

But this is a bad approach, I added a semaphore which is a cleaner approach

[joecummings]

Why do we want to try to run this for RECOVERING?

[allenwang28]

Good catch, I changed it so that it only runs for healthy. If it ever becomes not healthy, the loop stops (can't accept anymore requests) and re-spawns at recovery.

[joecummings]

Some small things to address, but overall lgtm

[joecummings]

kewl, I didn't know asyncio had this

[joecummings]

Dumb question, how would we get here? If the proc mesh creation errors, wouldn't it go to the exception immediately? And isn't that the only wait to have a none for self.proc_mesh?

[allenwang28]

not a dumb question - create_proc_mesh would raise the exception but this check is for if a downstream replica implementation (like Policy) doesn't do this correctly - this is another layer of guardrails. But with this current implementation, we will never hit it

[joecummings]

Would this be unhealthy or would it just be uninitialized?

[allenwang28]

hmm good question - uninitialized is like "the service just started and i haven't tried to init" vs unhealthy is like "the service tried to init but couldn't for some reason"

so in this context, unhealthy basically means "this thing is never going to initialize, you should look at your code", but right now I'm not doing anything. We can follow up when this inevitably becomes a problem

[joecummings]

Initialize with already set this state.

[joecummings]

Won't this be handled in initialize?

[allenwang28]

yeah this basically just adds another layer of logs saying "oh it failed during recovery, because it failed to initialize"

[joecummings]

Can't wait for this hidden gem to trip some people up haha

[joecummings]

What's the raw error if you let this through?

[allenwang28]

the service waits forever for a request that will never complete