We should allow the plugin to return a string explaining the reason for rejection. We can then just treat the empty string as the allow mechanism (less opinionated on this part tho).

Updated, thanks!

I think we may want to invert this. The attribute map is a component of the data system, and so we should refactor the data layer attribute map to fit our needs. The scheduling layer is informed by the data layer, so our structures should reflect that

++

update comment to reflect the fetch, and it only creates (with a default prio) should no infObjective be found

Principle of Least Surprise - Recommend returning the reqCtx here, generally we want to avoid modifying param structures without explicitly returning. And if we do need to modify the object in mem, our goDocs should specify that we do.

I think we do need to modify the reqCtx, copying it may be an overkill. Also, the comments of the method state it updates reqCtx. Please let me know if its unclear presently. Thanks!

I think the intention was NOT to copy, but to explicitly return the pointer.

Updated the function. Thanks!

Typically I am all for using prebuild libs to handle this type of complexity.

But since writes to specific attributes will lock the entire data object, we may have high lock contention here. Did we explore having a lock per attribute key?

That would let locks be at the granularity of a specific endpoint & a specific attribute, which should hopefully reduce lock contention & let our system be more performant.

Thanks for the suggestion. This attribute map is a per request copy where we take a snapshot of the attributes so that we can use them in the scheduling layer. Given the per request nature of the map, I think it won't have contention because it will take like t < microsecond to update the map. I think its reasonable to use sync map here.

Can we scale test to ensure we don't have any regression? We can consider baseline metrics what we have here: #1458

Can we have a test cast that functionally validates the intent of the plugin?

Currently we have a thin set of tests that basically just validate that we can successfully call a noop function.

The design doc detailed adding timeouts and error limits to these prepare data plugins as to not become a blocker in the event that the plugins are performing poorly, allowing the system to remain resilient. Was this considered in this PR?

I am planning to do that in a follow up PR. Keeping it simpler in the initial change to ensure the SLO aware routing work is unblocked and can use this. Also I think it would be simpler to comprehend/debug if we don't have retires/timeouts. Added TODO for now. Hope that's okay. Thanks!

Keeping it simpler in the initial change to ensure the SLO aware routing work is unblocked and can use this.

SLO aware routing is actually the use case I am concerned about, as it has known performance issues. In an effort to defend the systems current operations we need timeouts & retires to safely implement this functionality

I see. Updated the methods to make prepare data execute parallely for all plugins with timeout and retries. PTAL. Thanks!

can we remove logger from the arguments?
This comes back to a discussion we had on a different PR. the convention we use in the project is contextual logging, we take logger from context and use it instead of passing around logger as arg.
e.g.,

log.FromContext(ctx).V(logutil.VERBOSE).Info(...)

why do we duplicate this code from datalayer?

I'm bit confused by this PR.

we've added recently two interfaces for ProducerPlugin and ConsumerPlugin.
ProducerPlugin is a plugin that produces data.
why do we add another interface for a plugin the produces data?
shouldn't PrepareData function be on the same ProducerPlugin interface and validate that the produced data types are matching the types declaration?

Recommended reading: https://docs.google.com/document/d/1EQwXL2pCuUyM1B917FUgP_8pFS3VF8F_bUfjy8IE7gM/edit?tab=t.vmaefhinvkl5#heading=h.s9mr6kynb3ls

But the TL;DR is

Consume/Produce are expressions of intent only called on startup.

PrepareData is the actual generation of request specific data

right.
would it be correct to say that in order for a plugin to implement PrepareData it should always declare which data it would produce? meaning these two should always go together in the same interface?

if that’s not the case, can you give an example where one is used but the other isn’t?

Definitely, some data producers will still specify the data they produce, but not generate request specific data, and so do not need to implement PrepareData()

So while its true that an implementer of PrepareData() will always implement Produces(). Not all plugins that implement Produces() will implement PrepareData(). The Metrics scrapers are a great example of this

right. metrics collectors only produce, and not prepare per request.

an implementer of PrepareData() will always implement Produces()

I think the above should be forced, for example through the interface:

type PrepareData interface {
	plugins.ProducerPlugin //  < THIS IS THE CHANGE
	PrepareData(ctx context.Context, request *types.LLMRequest, pods []types.Pod)
}

as a side note - I think the naming selection is critical for readability.
PrepareData is too generic and doesn't say anything about a "per request".
it might be worth calling the function - PrepareRequestData (and rename the interface, e.g., DataProducer or something like that).

That all sounds reasonable to me

Updated. Thanks for the suggestions!

naming selection should be changed IMO.
more specifically, struct/interface names should be nouns, e.g., AdmissionPlugin, while function names should be verbs based, e.g., AdmitRequest.
additionally, the documentation should explain what happens when one is using multiple admission plugins (e.g., the request should pass all admission checks).

Updated, thanks

this comment is important. what is the relation between admission controller Admit and the admitRequest plugins?

++

This would be a break in contract of how flow control operates. This specific plugin is for request specific semantics. We have currently do not have Flow Control considering request specific semantics, and there hasnt been a proposal suggesting this change. I think we should remove this todo until we have strong reasoning to actually do this work.

my apologize, but I don't understand the intention of this new Admission plugin.
I thought we want to have admission check pluggable, but it seems now that we have two types of admission checks, with two different interfaces.
this seems wrong.

Removed the comment. Thanks for the catch!

why do we create snapshot of candidate pods?
we should work with candidatePods and create a snapshot only when calling the scheduler.
this is true for both prep data and admit request.
helper functions in the director should not rely on the internal scheduler representation of the endpoints.

both prep data and admit request are request specific, and so if we add request specific data to the shared endpoints that could risk data corruption.

Snapshotting before these steps ensures that this data lifecycle is only in the context it is consumed in.

the intention of converting the endpoints representation to scheduler internal structure was only for the purpose of sending it to the scheduler.
PodMetrics has MetricsState behind atomic pointer and reading the metrics is an atomic operation (read all metrics in one operation).
I must be missing something although I've read the proposal doc.

can you give an example of how AdmissionPlugin is using candidate pods?
IMO admission check should include only the request (including its metadata like headers) and the system state (like saturation or other system metrics).

@kfswain I really think we should think carefully on the interfaces and align on them.
it doesn't make sense to me to pass the candidate pods to admission check.

I would be happy to get an example for an admission check that is per request data and depends on the candidate pods (other than saturation that is checked separately).
I'm a bit concerned about over complication here, and unless there is a real use case for that we should probably wait with this addition (talking only about admission, data producer/consumer is clearly needed).

separately, I think there is a missing struct, like RequestState, that conceptually should be similar to PluginState we have today. RequestState should be created at the beginning of the request, and passed around for RequestDataProducers to be filled, and later on to ConsumerPlugins (probably instead of CycleState).

I was mostly following recommendation in https://docs.google.com/document/d/1EQwXL2pCuUyM1B917FUgP_8pFS3VF8F_bUfjy8IE7gM/edit?tab=t.vmaefhinvkl5#heading=h.5qzcvbilfn6d

I think we would be adding the first AdmitRequest plugin with latency predictor.

@kfswain please keep me honest here. Thanks!