-
Notifications
You must be signed in to change notification settings - Fork 17
Add support for running inference against the org endpoint #66
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.
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
Conversation
3096d1c to
68ce62d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables specifying an organization when running model inference by adding an --org flag, refactors Azure client configuration to separate root and path, and updates tests to accommodate the new method signatures.
- Introduce
--orgflag in run and eval commands and propagate it to the client - Refactor
AzureClientConfigto useInferenceRoot+InferencePathand updateGetChatCompletionStreamto build URLs with an optional org segment - Update mocks, interfaces, and tests for the new
orgparameter
Reviewed Changes
Copilot reviewed 36 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/azuremodels/unauthenticated_client.go | Added org parameter to stubbed streaming method |
| internal/azuremodels/types.go | Added Organization field to ChatCompletionOptions |
| internal/azuremodels/mock_client.go | Updated mock signature and implementation to include org |
| internal/azuremodels/client.go | Updated interface to accept org argument |
| internal/azuremodels/azure_client_test.go | Switched tests to use InferenceRoot and updated calls |
| internal/azuremodels/azure_client_config.go | Split default inference URL into root and path fields |
| internal/azuremodels/azure_client.go | Implemented org-aware URL construction for inference calls |
| cmd/run/run.go | Added --org flag, examples, and passed org to handler |
| cmd/run/run_test.go | Updated run command tests to expect org parameter |
| cmd/eval/eval.go | Added --org flag, examples, and passed org to handler |
| cmd/eval/eval_test.go | Updated eval tests to expect org parameter |
Comments suppressed due to low confidence (4)
cmd/run/run.go:416
- The help text for the
--orgflag is missing a closing parenthesis. Consider updating it to:"Organization to attribute usage to (omitting will attribute usage to the current actor)".
cmd.Flags().String("org", "", "Organization to attribute usage to (omitting will attribute usage to the current actor")
cmd/eval/eval.go:134
- The help text for the
--orgflag is missing a closing parenthesis. Consider updating it to:"Organization to attribute usage to (omitting will attribute usage to the current actor)".
cmd.Flags().String("org", "", "Organization to attribute usage to (omitting will attribute usage to the current actor")
internal/azuremodels/azure_client_test.go:52
- In tests,
InferencePathremains unset and defaults to empty, relying on the server handling/. For clarity and future compatibility, explicitly setInferencePath: defaultInferencePathin the test config.
cfg := &AzureClientConfig{InferenceRoot: testServer.URL}
internal/azuremodels/client.go:7
- The doc comment for
GetChatCompletionStreamshould be updated to reflect the neworgparameter in the method signature.
// GetChatCompletionStream returns a stream of chat completions using the given options.
3afdc58 to
a787251
Compare
This pull request introduces support for specifying an organization when running evaluations or inference against models, along with refactoring the Azure client configuration for improved flexibility. Key changes include adding the
--orgflag to relevant commands, updating the Azure client to dynamically construct inference URLs based on the organization, and modifying associated tests to accommodate the new functionality.