Skip to content

[Platform][OpenAI] Add region option #409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 4, 2025
Merged

[Platform][OpenAI] Add region option #409

merged 1 commit into from
Sep 4, 2025

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Sep 2, 2025 •
edited
Loading

Q A
Bug fix? no
New feature? yes
Docs? no
Issues Fix #407
License MIT

@OskarStark OskarStark self-assigned this Sep 2, 2025
@OskarStark OskarStark added the Platform Issues & PRs about the AI Platform component label Sep 2, 2025
@OskarStark OskarStark mentioned this pull request Sep 2, 2025
Closed
@OskarStark OskarStark changed the title [Platform][OpenAI] Add region option support [Platform][OpenAI] Add region option Sep 2, 2025
@OskarStark
Copy link
Contributor Author

Ready to merge

@chr-hertel
Copy link
Member

two things:

  • i think it would be more consistent to use $region in the ModelClient constructors as well - with the same argument as you used in the issue
  • what do you think about a BaseModelClient that encapsulates that base url handling, and all children could just call getBaseUrl() of the base implementation - taking care of the region handling

@OskarStark
Copy link
Contributor Author

i think it would be more consistent to use $region in the ModelClient constructors as well - with the same argument as you used in the issue

makes sense, lets do it

what do you think about a BaseModelClient that encapsulates that base url handling, and all children could just call getBaseUrl() of the base implementation - taking care of the region handling

I will have a look, but as a follow up then

petski
petski reviewed Sep 3, 2025
View reviewed changes
@OskarStark OskarStark force-pushed the open-ai-platform branch 2 times, most recently from cee3e39 to 1f3501c Compare September 3, 2025 08:17
petski
petski reviewed Sep 3, 2025
View reviewed changes
petski
petski reviewed Sep 3, 2025
View reviewed changes
@OskarStark OskarStark marked this pull request as draft September 3, 2025 08:49
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 3, 2025
View reviewed changes
@OskarStark OskarStark force-pushed the open-ai-platform branch 2 times, most recently from 217af4e to 5df37f6 Compare September 3, 2025 12:20
OskarStark
OskarStark commented Sep 4, 2025
View reviewed changes
src/platform/src/Bridge/OpenAi/Embeddings/ModelClient.php Outdated
* @author Christopher Hertel <mail@christopher-hertel.de>
*/
final readonly class ModelClient implements PlatformResponseFactory
final readonly class ModelClient extends AbstractModelClient implements PlatformResponseFactory
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chr-hertel in src/platform/src/Bridge/OpenAi/DallE/ModelClient.php we do not alias the ModelClientInterface to PlatformResponseFactory. Shall we streamline this?

Copy link
Member

Choose a reason for hiding this comment

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

i'm afraid the PlatformResponseFactory is rather a artifact from old times and is not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@OskarStark OskarStark marked this pull request as ready for review September 4, 2025 05:33
@OskarStark
Copy link
Contributor Author

Ready to merge from my side @chr-hertel @petski

petski
petski reviewed Sep 4, 2025
View reviewed changes
petski
petski reviewed Sep 4, 2025
View reviewed changes
OskarStark
OskarStark commented Sep 4, 2025
View reviewed changes
chr-hertel
chr-hertel approved these changes Sep 4, 2025
View reviewed changes
Copy link
Member

@chr-hertel chr-hertel left a comment

Choose a reason for hiding this comment

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

In general you could pull more logic up into the abstract implementation, but the region stuff was the important part to me at this point

@OskarStark OskarStark merged commit bb6b953 into symfony:main Sep 4, 2025
7 checks passed
@OskarStark OskarStark deleted the open-ai-platform branch September 4, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chr-hertel chr-hertel chr-hertel approved these changes

@Nyholm Nyholm Awaiting requested review from Nyholm Nyholm is a code owner

+1 more reviewer

@petski petski petski left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@OskarStark OskarStark

Labels

Feature New feature Platform Issues & PRs about the AI Platform component Status: Reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Endpoint URL for api.openai.com hardcoded

4 participants

@OskarStark @chr-hertel @petski @carsonbot