feat: separate and optimize async and sync clients

[judahrand]

What does this PR address?

A draft attempt at addressing #4115

Currently, this introduces a lot of duplicate code. However, I think it should be possible to reduce this significantly.

I believe by having separate sync and async implementations optimal performance can be achieved for both.

I'd be interested to get thoughts and feedback on this PR as well as advice on how to benchmark it against main as I believe it should see some performance benefit across multiple requests on the same client.

Fixes #(issue)

Before submitting:

• Does the Pull Request follow Conventional Commits specification naming? Here are GitHub's
  guide on how to create a pull request.
• Does the code follow BentoML's code style, pre-commit run -a script has passed (instructions)?
• Did you read through contribution guidelines and follow development guidelines?
• Did your changes require updates to the documentation? Have you updated
  those accordingly? Here are documentation guidelines and tips on writting docs.
• Did you write tests to cover your changes?

[sauyon]

For backwards compatibility, the client implementations will have to retain the sync/async methods; perhaps the default client could instantiate a sync / async client?

[judahrand]

For backwards compatibility, the client implementations will have to retain the sync/async methods; perhaps the default client could instantiate a sync / async client?

@sauyon The latest changes should maintain backwards compatibility. Is this sort of a solution acceptable? If so I'll push forwards with getting the tests passing and perhaps also look at splitting the tests up by sync and async implementation.

[judahrand]

I think that this PR has gotten a bit muddled. This is something which I am keen to address as at the moment I think the client implementations are quite poor. However, I may take another stab at this later understanding more about all the various moving parts.

There seems to be a lot which needs changes to:

• make the synchronous clients work reliably and efficiently
• make the async clients work efficiently
• separate the client implementation from the core BentoML package to avoid carrying huge numbers of irrelevant dependencies around in client side software.

[sauyon]

A few things that I think need changing. Haven't reviewed the async client code entirely carefully yet.

@aarnphm can you help review the grpc client?

[sauyon]

I think that this PR has gotten a bit muddled. This is something which I am keen to address as at the moment I think the client implementations are quite poor. However, I may take another stab at this later understanding more about all the various moving parts.

I think apart from the IO descriptor change we should get this in, then probably circle back after we rework those.

There seems to be a lot which needs changes to:

• make the synchronous clients work reliably and efficiently
• make the async clients work efficiently

Yep.

Somewhat related, there was at one point an issue where connections were being dropped and we were running into strange errors when we reused a connection which we haven't gotten to the bottom of, which I probably could have added a comment for, but we really need to figure that out.

• separate the client implementation from the core BentoML package to avoid carrying huge numbers of irrelevant dependencies around in client side software.

We probably need to think more carefully about this one in general; a lot of our dependencies are only required for servers / only required for clients, etc. @parano @bojiang something to think about, I think.

[aarnphm]

FWIW I think we can separate out a bentoml-client package, and by default bentoml will include bentoml-client

[judahrand]

FWIW I think we can separate out a bentoml-client package, and by default bentoml will include bentoml-client

Agreed. I also wonder if the serialization/deserialization logic should live in a separate package (given that both the client and server will want to depend on it). This is something a lot of Rust projects do very very well (see arrow-rs). They split out all their subcomponents into sub crates.

I believe PDM might even be able to cope with this setup in Python? https://pdm.fming.dev/latest/usage/advanced/#use-pdm-to-manage-a-monorepo

[aarnphm]

FWIW I think we can separate out a bentoml-client package, and by default bentoml will include bentoml-client

Agreed. I also wonder if the serialization/deserialization logic should live in a separate package (given that both the client and server will want to depend on it). This is something a lot of Rust projects do very very well (see arrow-rs). They split out all their subcomponents into sub crates.

I believe PDM might even be able to cope with this setup in Python? pdm.fming.dev/latest/usage/advanced/#use-pdm-to-manage-a-monorepo

this will be added to bentoml-core 🎉

[aarnphm]

@ssheng do you think it is good to refactor our repo into monorepo architecture now?

Maybe good to also separate SDK and CLI as well as client

We might also want to think about bentoml-core development 🤔

[sauyon]

I don't know that it needs to be a monorepo.

[sauyon]

Would it be ok if I took this one over so we can get it in while we work on the rework?

[judahrand]

Would it be ok if I took this one over so we can get it in while we work on the rework?

100% - sorry for letting this one lag. Various demands on time etc etc etc - same as everyone else 🤣

[frostming]

I am mostly fine with it.

[sauyon]

@aarnphm @frostming should be ready for another look!