Skip to content

api: adds AllowModeOverride for extproc#5099

Merged
arkodg merged 3 commits intoenvoyproxy:mainfrom
mathetake:modeoverrides
Jan 21, 2025
Merged

api: adds AllowModeOverride for extproc#5099
arkodg merged 3 commits intoenvoyproxy:mainfrom
mathetake:modeoverrides

Conversation

@mathetake
Copy link
Copy Markdown
Member

@mathetake mathetake commented Jan 20, 2025

What type of PR is this?

Adds a new API for external processor

What this PR does / why we need it:

This adds AllowModeOverride boolean config
to the external processing config.

Which issue(s) this PR fixes:

N/A

@mathetake
api: adds AllowModeOverride for extproc
2600b2d 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake requested a review from a team as a code owner January 20, 2025 04:13
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 20, 2025

@arkodg @zirain @zhaohuabing ptal

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 20, 2025

sorry i haven't added the test yet - just wanted to know if this is ok before then

zirain
zirain previously approved these changes Jan 20, 2025
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.85%. Comparing base (4d560a2) to head (5265c8f).
Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5099      +/-   ##
==========================================
- Coverage   66.87%   66.85%   -0.03%     
==========================================
  Files         210      210              
  Lines       32953    32948       -5     
==========================================
- Hits        22038    22027      -11     
- Misses       9585     9589       +4     
- Partials     1330     1332       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mathetake
add test
60180d3 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 20, 2025

added (obvious) test

@mathetake mathetake requested a review from zirain January 20, 2025 18:02
zhaohuabing
zhaohuabing previously approved these changes Jan 21, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@guydc
Copy link
Copy Markdown
Contributor

guydc commented Jan 21, 2025

+1 in general. Maybe this can be grouped in the "processing mode" container. e.g. processingMode.allowOverride=true.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jan 21, 2025

hey @mathetake @guydc can you share the use case for this ? is it when the policy creator is unaware of what the processing mode should be and is set lazily by the ext proc service ?

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 21, 2025 •
edited
Loading

when dealing with OpenAI and any other AI chat completion endpoints, the returned content is conditionally streaming - the chat completion is sent line by line. so we are converting the response line by line and want to send them to the client as soon as possible not to block the giant entire events.

See
https://github.com/envoyproxy/ai-gateway/blob/d851a65d75de10b7de64a92c275c236959625511/internal/extproc/translator/openai_awsbedrock.go#L50-L57

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jan 21, 2025

thanks @mathetake this makes sense
prefer @guydc's API suggestion processingMode.allowOverride

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 21, 2025

sounds good, will rework the pr then

@mathetake
review: move it to inside processingMode
5265c8f 
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
@mathetake mathetake requested a review from zhaohuabing January 21, 2025 19:33
@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 21, 2025

@arkodg done

@mathetake
Copy link
Copy Markdown
Member Author

mathetake commented Jan 21, 2025

no idea why the doc build is failing (seems irrelevant)

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jan 21, 2025

maybe related to linkinator thats working hard to make sure link references are working

2025-01-21T19:34:08.9352640Z [500] https://jwt.io/

arkodg
arkodg approved these changes Jan 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team January 21, 2025 20:04
@arkodg arkodg added this to the v1.3.0-rc.1 milestone Jan 21, 2025
@arkodg arkodg merged commit ebdba31 into envoyproxy:main Jan 21, 2025
@mathetake mathetake mentioned this pull request Jan 21, 2025
Merged
@mathetake mathetake deleted the modeoverrides branch January 21, 2025 23:20
EshaanAgg pushed a commit to EshaanAgg/gateway that referenced this pull request Jan 22, 2025
@mathetake @EshaanAgg
api: adds AllowModeOverride for extproc (envoyproxy#5099)
75e0f0d 
* api: adds AllowModeOverride for extproc

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

* add test

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

* review: move it to inside processingMode

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

---------

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Signed-off-by: EshaanAgg <96648934+EshaanAgg@users.noreply.github.com>
mathetake added a commit to envoyproxy/ai-gateway that referenced this pull request Jan 24, 2025
@mathetake
controller: set allowModeOverride=true (#158)
7a66d51 
This allows us to set allowModeOverride=true
for our external processor. The flag is needed
to deal with the stream=true requests.
This is introduced to EG today: envoyproxy/gateway#5099

---------

Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg arkodg approved these changes

@guydc guydc guydc approved these changes

@zirain zirain Awaiting requested review from zirain

@zhaohuabing zhaohuabing Awaiting requested review from zhaohuabing

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.3.0-rc.1

Development

Successfully merging this pull request may close these issues.

5 participants

@mathetake @guydc @arkodg @zhaohuabing @zirain