Design Discussion: Async Support?

[martindevans]

Currently LLamaSharp is in a bit of an awkward position with async support.

• ILLamaExecutor specifies two infer methods, one async and one non-async.
• All implementations of InferAsync currently just call Infer, which is useless!
• Inference itself is not really async (although we could make it act like it).
• There are some things LLamaSharp does which could be async (e.g. saving/loading session files), but they're not because they're implemented in the non-async Infer method. Making these async would require duplicating a huge amount of inference code.

So we're in the position that we pretend to have async, but don't actually have async, and the small amount of work we have which could be async isn't!

Option 1: Remove Async

Right now the library is not really async, the small amount of IO work that is in the library isn't async and all inference computations block. So we could just remove async support altogether and lose nothing.

Option 2: Remove Non-Async

The other option is to remove non async support. That way the inference code would all be in an async context - any IO work would be easy to make properly async and the expensive computations (i.e. Eval()) could be done in a Task.Run call and awaited to not block the calling thread.

---

Personally I prefer option 2. But I realise that requiring async may not be preferred by everyone! Thoughts?

[mingyangzhu]

Support Option 2: Remove Non-Async

[saddam213]

Vote option 2

IAsyncEnumearabe has extenseins for syncronous Enumerable ToBlockingEnumerable in the System.Linq.Async package if I remember correctly

So if the default infer was Async then we can easily give users an sync version with a simple Infer().ToBlockingEnumerable()

So if we reversed implemetation like you suggest we can support both

Using that method in my lib to give users a sync version
https://github.com/saddam213/LLamaStack/blob/master/LLamaStack.WebApi/Models/InferResponseBase.cs

[drasticactions]

I agree for option 2, it's easy enough to make the async version non-async if you want to, and having one implmentation that does the things it says makes sense to me.

[SignalRT]

Support Option 2: Remove Non-Async

[martindevans]

Is it safe to convert async into non async with something like ToBlockingEnumerable? See this answer by Eric Lippert for example: https://stackoverflow.com/questions/45034554/how-to-correctly-block-on-async-code

[saddam213]

Is it safe to convert async into non async with something like ToBlockingEnumerable? See this answer by Eric Lippert for example: https://stackoverflow.com/questions/45034554/how-to-correctly-block-on-async-code

If your worried about the .NET implemetation of ToBlockingEnumerable just do the Async stuff, any end user can use the extension method themselfs in thier own app

No need for LLamaSharp to explicitly provide it

Morning Coffee Edit:
I also don't think the async iterator has the same pitfalls as calling Task.Result etc, as the loop itself is not pallalel but synchronous, its the execution of body that async.

I would still lean towards adding it purely to help the end users that dont know much about the TPL or IAsyncEnumearble

There are legitimate reasons to have an sync environment still, maybe, and dealing with IAsyncEnumerable can be tricky in that area, ToBlockingEnumerable will stop end users trying to iterate it themselves synchronously, its also has a few other pros like delayed execution so it will behave like the async version.

Adding the package itself will introduce the end users to Async.Linq IAsyncEnumearble extensions, Count, Where, ToListAsync so that could also be an added bonus for the usability of the LLamaSharp nuget package

[martindevans]

I've opened PR #173 with the minimal changes to make things async-only, so we can see what this would look like.

There's no sync version for now, but if we do want that it can easily be added an an extension method to ILLamaExecutor.