Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughSupport for the Ollama AI provider has been added across the codebase, including implementation, configuration, documentation, and testing. The Mistral provider is also now documented as supported. These changes update provider lists, schemas, configuration comments, and introduce a new Ollama provider implementation and test. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bifrost
participant OllamaProvider
participant OllamaAPI
Client->>Bifrost: Request chat completion (provider=Ollama)
Bifrost->>OllamaProvider: ChatCompletion(...)
OllamaProvider->>OllamaAPI: POST /api/chat with messages
OllamaAPI-->>OllamaProvider: JSON response
OllamaProvider-->>Bifrost: BifrostResponse
Bifrost-->>Client: Response
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (12)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
README.md(4 hunks)core/bifrost.go(3 hunks)core/providers/ollama.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(1 hunks)core/tests/account.go(3 hunks)core/tests/mistral_test.go(0 hunks)core/tests/ollama_test.go(1 hunks)docs/http-transport-api.md(2 hunks)docs/openapi.json(2 hunks)docs/providers.md(1 hunks)transports/bifrost-http/main.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/tests/mistral_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/tests/account.go (2)
core/schemas/bifrost.go (9)
ModelProvider(36-36)OpenAI(39-39)Anthropic(41-41)Bedrock(42-42)Cohere(43-43)Azure(40-40)Vertex(44-44)Mistral(45-45)Ollama(46-46)core/schemas/provider.go (4)
ProviderConfig(108-115)NetworkConfig(29-36)DefaultRequestTimeoutInSeconds(13-13)ConcurrencyAndBufferSize(72-75)
🪛 LanguageTool
README.md
[style] ~192-~192: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “API”.
Context: ...Ollama, etc.). It provides a consistent API interface while handling: - Authentication and k...
(ACRONYM_TAUTOLOGY)
[style] ~353-~353: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...cumentation improvements, or new ideas. Feel free to open an issue, and once its' assigned, ...
(FEEL_FREE_TO_STYLE_ME)
[uncategorized] ~353-~353: “its” (belonging to it) seems less likely than “it’s” (it is)
Context: ...s. Feel free to open an issue, and once its' assigned, submit a Pull Request. Here...
(AI_HYDRA_LEO_CPT_ITS_ITIS)
🔇 Additional comments (10)
transports/bifrost-http/main.go (1)
2-2: Documentation update: providers list includes new entries
Listing updated to include "Mistral" and "Ollama", aligning with new provider support.docs/providers.md (1)
15-16: Providers list updated to include Mistral and Ollama
Documentation now reflects the addition of the new model providers.core/schemas/bifrost.go (1)
46-46: AddOllamatoModelProviderenumeration
The new provider constantOllamaaligns with the implementation incore/providers/ollama.goand ensures consistent schema support.README.md (2)
206-206: Confirm features list includes new providers
The features section correctly lists Mistral and Ollama under multi-provider support.
323-323: Align note formatting
The added note clarifying overhead measurement on t3.xlarge is clear and well-formatted.docs/http-transport-api.md (2)
230-235: Update CompletionRequest provider examples
Theproviderfield description correctly now includesmistralandollama.
454-455: Supported Providers table includes new entries
The Supported Providers table at the end has been updated to list Mistral and Ollama.core/bifrost.go (2)
143-145: Happy path – provider wired in correctly
Ollamacase added and wired toproviders.NewOllamaProvider, matching the existing pattern. 👍
375-377: Empty key forwarded to provider – double-check Ollama implementation
keyremains""for Ollama. Ensureproviders.NewOllamaProvider(or itsChatCompletion) treats an empty key as expected and does not accidentally forward anAuthorization:header with a blank value.core/tests/ollama_test.go (1)
21-24: Model name may not match real tag
"llama3.2"looks like a typo (Ollama images are typicallyllama3:latestor similar). If this tag is wrong the test will silently fail when someone runs it against a real Ollama instance.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Pratham-Mishra04
force-pushed
the
06-14-feat_mistral_provider_added
branch
from
c62001a to
7a2e3ac
Compare
Pratham-Mishra04
force-pushed
the
06-14-feat_ollama_provider_added
branch
from
d5646ab to
2428564
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
core/bifrost.go (1)
178-181: 🛠️ Refactor suggestionCompilation error: ranging over an int
rangecannot iterate over anint. This will not compile.- for range providerConfig.ConcurrencyAndBufferSize.Concurrency { + for i := 0; i < providerConfig.ConcurrencyAndBufferSize.Concurrency; i++ {
♻️ Duplicate comments (7)
core/schemas/provider.go (1)
30-30: Fix grammar & spacing in BaseURL comment (repeat)
The wording is still hard to parse and the space before “(required here)” is missing – same feedback as the previous review.- // BaseURL is only supported for OpenAI, Anthropic, Cohere, Mistral, and Ollama(required here) providers + // BaseURL is supported for OpenAI, Anthropic, Cohere, Mistral, and Ollama providers + // (required for Ollama).docs/openapi.json (2)
5-5: Long provider sentence still unreadable (repeat)
The description is getting unwieldy; please convert the provider list to bullet-points or otherwise make it easier to scan, as suggested earlier.
435-444: Provider enum order inconsistency (repeat)
vertexsits betweencohereandmistral; the rest is alphabetical. Adopt one ordering rule and keep it consistent across specs & code.core/tests/account.go (1)
225-232: Fail fast if OLLAMA_BASE_URL is missing (repeat)
BaseURLis required, but an empty env var silently passes through. Add a guard to panic or return an error whenOLLAMA_BASE_URLisn’t set.core/providers/ollama.go (3)
63-66:MaxConnsPerHostshould matchConcurrency, notBufferSize.This was pointed out earlier but is still unfixed – the pool size will be limited by
BufferSize, while the rest of the code throttles onConcurrency. Use the same knob for consistency.- MaxConnsPerHost: config.ConcurrencyAndBufferSize.BufferSize, + MaxConnsPerHost: config.ConcurrencyAndBufferSize.Concurrency,
68-72: Compile-time error – cannot range over anint.
for range config.ConcurrencyAndBufferSize.Concurrencydoes not compile.- for range config.ConcurrencyAndBufferSize.Concurrency { + for i := 0; i < config.ConcurrencyAndBufferSize.Concurrency; i++ {
131-134: Skip emptyAuthorizationheader.Ollama usually runs without authentication; sending
Authorization: Beareris unnecessary and might break proxies.- req.Header.Set("Authorization", "Bearer "+key) + if key != "" { + req.Header.Set("Authorization", "Bearer "+key) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
README.md(4 hunks)core/bifrost.go(3 hunks)core/providers/ollama.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(1 hunks)core/tests/account.go(3 hunks)core/tests/mistral_test.go(0 hunks)core/tests/ollama_test.go(1 hunks)docs/http-transport-api.md(2 hunks)docs/openapi.json(2 hunks)docs/providers.md(1 hunks)transports/bifrost-http/main.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/tests/mistral_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
core/tests/account.go (2)
core/schemas/bifrost.go (9)
ModelProvider(36-36)OpenAI(39-39)Anthropic(41-41)Bedrock(42-42)Cohere(43-43)Azure(40-40)Vertex(44-44)Mistral(45-45)Ollama(46-46)core/schemas/provider.go (4)
ProviderConfig(108-115)NetworkConfig(29-36)DefaultRequestTimeoutInSeconds(13-13)ConcurrencyAndBufferSize(72-75)
🪛 LanguageTool
README.md
[style] ~192-~192: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “API”.
Context: ...Ollama, etc.). It provides a consistent API interface while handling: - Authentication and k...
(ACRONYM_TAUTOLOGY)
[style] ~353-~353: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...cumentation improvements, or new ideas. Feel free to open an issue, and once its' assigned, ...
(FEEL_FREE_TO_STYLE_ME)
[uncategorized] ~353-~353: “its” (belonging to it) seems less likely than “it’s” (it is)
Context: ...s. Feel free to open an issue, and once its' assigned, submit a Pull Request. Here...
(AI_HYDRA_LEO_CPT_ITS_ITIS)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (10)
transports/bifrost-http/main.go (1)
2-2: Update provider list in package comment
The package comment now accurately reflects support for Mistral and Ollama alongside existing providers.docs/providers.md (1)
15-16: Add Mistral and Ollama to supported providers list
The documentation correctly includes the new providers, aligning with code and other docs.core/schemas/bifrost.go (1)
46-46: Register Ollama in ModelProvider enum
The newOllamaconstant is added with the proper lowercase string value, matching the pattern for other providers.README.md (3)
206-206: Approve: Feature list updated
The features section now correctly lists Mistral and Ollama as supported providers.
321-321: Approve: Benchmark footnote added
The overhead measurement footnote for t3 instances is clear and informative.
323-323: Approve: Note on xlarge performance
The additional note on t3.xlarge response parsing improvements is valuable.docs/http-transport-api.md (1)
232-232: Approve: Include Mistral and Ollama in schema examples
Theproviderfield description now listsmistralandollama, matching the updated enum.core/bifrost.go (2)
143-145: Ollama provider wiring looks good
Creation path matches other providers and returns both values fromNewOllamaProvider.
375-377: Key selection correctly skipped for vertex/ollama
Logic is sound; both providers operate without API keys.core/tests/ollama_test.go (1)
21-27: Verify feature flags vs provider capabilities
SetupImage,SetupBaseImage, andSetupToolCallsare enabled, but Ollama currently exposes only chat completions. Confirm these helpers are no-ops for unsupported endpoints or the test will fail.
Pratham-Mishra04
force-pushed
the
06-14-feat_ollama_provider_added
branch
from
2428564 to
71791a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
core/schemas/provider.go (1)
30-30: 🧹 Nitpick (assertive)Clean up comment grammar for
BaseURLfield
The comment is lengthy and merges provider list with requirement detail; consider splitting into two sentences for clarity.Proposed diff:
- // BaseURL is supported for OpenAI, Anthropic, Cohere, Mistral, and Ollama providers (required for Ollama) + // BaseURL is supported for OpenAI, Anthropic, Cohere, Mistral, and Ollama providers. + // (Required for Ollama.)core/providers/ollama.go (3)
62-66: Mismatch between connection-pool size and concurrency setting
MaxConnsPerHostis configured withBufferSize, but all throttling / pool pre-warm logic elsewhere relies onConcurrency.
UnlessBufferSizeis intentionally distinct, this will silently allow more simultaneous connections than the concurrency guard anticipates.- MaxConnsPerHost: config.ConcurrencyAndBufferSize.BufferSize, + MaxConnsPerHost: config.ConcurrencyAndBufferSize.Concurrency,
156-162: Object is returned to the pool before the caller can use it
resultis released in adeferthat runs before the function returns its pointer to the caller, leaving the caller with a struct that may already be re-used by another goroutine.- result := acquireBifrostResponse() - defer releaseBifrostResponse(result) + // Caller must release the object after use to avoid data races. + result := acquireBifrostResponse()
170-175: Returned slice aliasing freed memory – deep-copy is required
result.Choicesdirectly aliasesresponse.Choices, yetresponseis freed right afterwards.
Any subsequent pool reuse may mutate the memory backingresult.Choices, causing data corruption.- result.Choices = response.Choices + result.Choices = make([]schemas.BifrostResponseChoice, len(response.Choices)) + copy(result.Choices, response.Choices)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
README.md(5 hunks)core/bifrost.go(3 hunks)core/providers/ollama.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(1 hunks)core/tests/account.go(3 hunks)core/tests/mistral_test.go(0 hunks)core/tests/ollama_test.go(1 hunks)docs/http-transport-api.md(2 hunks)docs/openapi.json(2 hunks)docs/providers.md(1 hunks)transports/bifrost-http/main.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/tests/mistral_test.go
🧰 Additional context used
🧠 Learnings (1)
core/providers/ollama.go (3)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: core/providers/ollama.go:68-72
Timestamp: 2025-06-15T14:31:59.613Z
Learning: Go 1.23 introduced the ability to range over integers. The syntax `for range n` where n is an integer will iterate from 0 to n-1, making `for range config.ConcurrencyAndBufferSize.Concurrency` valid syntax for iterating a specific number of times without needing an explicit counter variable.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: core/providers/ollama.go:68-72
Timestamp: 2025-06-15T14:31:59.613Z
Learning: Go 1.22 introduced the ability to range over integers. The syntax `for range n` where n is an integer will iterate from 0 to n-1, making `for range config.ConcurrencyAndBufferSize.Concurrency` valid syntax for iterating a specific number of times without needing an explicit counter variable. This feature requires Go module version 1.22 or later.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: core/providers/ollama.go:59-61
Timestamp: 2025-06-15T14:34:29.390Z
Learning: The `CheckAndSetDefaults()` method on `*schemas.ProviderConfig` in Go does not return any error - it has a void return type and only sets default values on the configuration struct.
🧬 Code Graph Analysis (3)
core/tests/ollama_test.go (3)
core/tests/tests.go (2)
TestConfig(34-46)SetupAllRequests(347-391)core/schemas/provider.go (1)
Provider(144-151)core/schemas/bifrost.go (1)
Ollama(46-46)
core/tests/account.go (2)
core/schemas/bifrost.go (9)
ModelProvider(36-36)OpenAI(39-39)Anthropic(41-41)Bedrock(42-42)Cohere(43-43)Azure(40-40)Vertex(44-44)Mistral(45-45)Ollama(46-46)core/schemas/provider.go (4)
ProviderConfig(108-115)NetworkConfig(29-36)DefaultRequestTimeoutInSeconds(13-13)ConcurrencyAndBufferSize(72-75)
core/providers/ollama.go (3)
core/schemas/bifrost.go (10)
BifrostResponseChoice(394-400)LLMUsage(302-308)BifrostResponse(289-299)ModelProvider(36-36)Ollama(46-46)ModelParameters(82-96)BifrostError(417-423)ErrorField(426-433)BifrostMessage(200-208)BifrostResponseExtraFields(403-410)core/schemas/logger.go (1)
Logger(18-35)core/schemas/provider.go (7)
ProviderConfig(108-115)NetworkConfig(29-36)DefaultRequestTimeoutInSeconds(13-13)ConcurrencyAndBufferSize(72-75)ProxyConfig(98-103)ErrProviderJSONMarshaling(22-22)Provider(144-151)
🪛 LanguageTool
README.md
[style] ~353-~353: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...cumentation improvements, or new ideas. Feel free to open an issue, and once it's assigned, ...
(FEEL_FREE_TO_STYLE_ME)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (9)
transports/bifrost-http/main.go (1)
2-2: Include Mistral and Ollama in supported providers list
Documentation update correctly extends the list of AI providers in the HTTP transport.docs/providers.md (1)
15-16: Document new providers: Mistral and Ollama
The supported providers list in the documentation now includes the newly added Mistral and Ollama entries, reflecting the core changes.core/schemas/bifrost.go (1)
46-46: AddOllamatoModelProviderenumeration
TheOllamaconstant has been introduced, ensuring the new provider is recognized in the Bifrost schema.docs/http-transport-api.md (2)
230-232: Expand provider enumeration inCompletionRequestschema
Theproviderfield documentation now lists the newmistralandollamaoptions, keeping the API reference in sync.
454-455: Document Mistral and Ollama in supported providers table
The supported providers table has been updated to include Mistral and Ollama keys.docs/openapi.json (2)
5-6: No issues – description update looks good
mistralandollamaare correctly appended in the description sentence; wording and casing are consistent with the existing style.
435-444: Enum extension is consistent with provider constant list
The new values are appended without disrupting downstream tooling that may rely on the previous order. 👍README.md (1)
192-214: Documentation tweaks land well
The “API interface” tautology and “Built-in Observability” typo are fixed and the new providers are listed – all good.core/tests/account.go (1)
30-39: Provider list now exhaustive – nice
All supported providers are returned; downstream initialisation will succeed without manual tweaks.
Pratham-Mishra04
force-pushed
the
06-14-feat_ollama_provider_added
branch
from
71791a0 to
511dcc0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
core/schemas/provider.go (1)
30-30: 🧹 Nitpick (assertive)Refine and clarify
BaseURLcomment formatting
Split the long sentence and improve readability per existing guidelines.-// BaseURL is supported for OpenAI, Anthropic, Cohere, Mistral, and Ollama providers (required for Ollama) +// BaseURL is supported for OpenAI, Anthropic, Cohere, and Mistral providers. +// For Ollama, BaseURL is required.core/tests/ollama_test.go (2)
15-16: Unreachablereturnis still here
Same observation as in the previous round – thereturnaftert.Fatalfis never executed.
12-18:⚠️ Potential issueSkip test when
OLLAMA_BASE_URLis not set to avoid CI breakage.Running this test on machines that don’t have a local Ollama instance will fail during provider construction.
Guard it so the suite remains green in shared CI environments.func TestOllama(t *testing.T) { bifrost, err := getBifrost() if err != nil { t.Fatalf("Error initializing bifrost: %v", err) return } + if os.Getenv("OLLAMA_BASE_URL") == "" { + t.Skip("Skipping Ollama integration test: OLLAMA_BASE_URL not set") + }core/tests/account.go (1)
224-233:⚠️ Potential issueFail fast when
OLLAMA_BASE_URLis missing.
NewOllamaProviderpanics whenBaseURLis empty, so returning an empty string here pushes the failure downstream.
Make the absence explicit and immediately visible:- BaseURL: os.Getenv("OLLAMA_BASE_URL"), + BaseURL: func() string { + v := os.Getenv("OLLAMA_BASE_URL") + if v == "" { + panic("OLLAMA_BASE_URL must be set for Ollama tests") + } + return v + }(),core/providers/ollama.go (1)
157-176:⚠️ Potential issueObject returned to pool before caller can use it & slice aliasing – data-race hazard
defer releaseBifrostResponse(result)returnsresultto the pool as soon asChatCompletionreturns, so the caller reads recycled memory.result.Choices = response.Choiceshands the caller a slice that aliasesresponse. That slice’s backing array is also recycled immediately after the deferredreleaseOllamaResponse(response)executes.- response := acquireOllamaResponse() - defer releaseOllamaResponse(response) - - result := acquireBifrostResponse() - defer releaseBifrostResponse(result) + response := acquireOllamaResponse() + // response will be released after a deep-copy + + result := acquireBifrostResponse() // caller is now responsible for releasing ... - result.Choices = response.Choices + result.Choices = make([]schemas.BifrostResponseChoice, len(response.Choices)) + copy(result.Choices, response.Choices) + + // Safe to release after deep copy + releaseOllamaResponse(response)Failing to deep-copy and deferring the pool release will cause elusive corruption in concurrent workloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
README.md(5 hunks)core/bifrost.go(4 hunks)core/providers/ollama.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/provider.go(1 hunks)core/tests/account.go(3 hunks)core/tests/mistral_test.go(0 hunks)core/tests/ollama_test.go(1 hunks)docs/http-transport-api.md(2 hunks)docs/openapi.json(2 hunks)docs/providers.md(1 hunks)transports/bifrost-http/main.go(1 hunks)
💤 Files with no reviewable changes (1)
- core/tests/mistral_test.go
🧰 Additional context used
🧠 Learnings (1)
core/providers/ollama.go (4)
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: core/providers/ollama.go:68-72
Timestamp: 2025-06-15T14:31:59.613Z
Learning: Go 1.23 introduced the ability to range over integers. The syntax `for range n` where n is an integer will iterate from 0 to n-1, making `for range config.ConcurrencyAndBufferSize.Concurrency` valid syntax for iterating a specific number of times without needing an explicit counter variable.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: core/providers/ollama.go:68-72
Timestamp: 2025-06-15T14:31:59.613Z
Learning: Go 1.22 introduced the ability to range over integers. The syntax `for range n` where n is an integer will iterate from 0 to n-1, making `for range config.ConcurrencyAndBufferSize.Concurrency` valid syntax for iterating a specific number of times without needing an explicit counter variable. This feature requires Go module version 1.22 or later.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: core/providers/ollama.go:59-61
Timestamp: 2025-06-15T14:34:29.390Z
Learning: The `CheckAndSetDefaults()` method on `*schemas.ProviderConfig` in Go does not return any error - it has a void return type and only sets default values on the configuration struct.
Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:96-101
Timestamp: 2025-06-15T13:50:41.379Z
Learning: In the Bifrost project, when a provider doesn't support a specific operation (like text completion), the IsBifrostError flag should be set to false to mark it as a provider-level error rather than a Bifrost framework error. This is intentional design for unsupported operations.
🧬 Code Graph Analysis (3)
core/tests/ollama_test.go (3)
core/tests/tests.go (2)
TestConfig(34-46)SetupAllRequests(347-391)core/schemas/provider.go (1)
Provider(144-151)core/schemas/bifrost.go (1)
Ollama(46-46)
core/tests/account.go (2)
core/schemas/bifrost.go (9)
ModelProvider(36-36)OpenAI(39-39)Anthropic(41-41)Bedrock(42-42)Cohere(43-43)Azure(40-40)Vertex(44-44)Mistral(45-45)Ollama(46-46)core/schemas/provider.go (4)
ProviderConfig(108-115)NetworkConfig(29-36)DefaultRequestTimeoutInSeconds(13-13)ConcurrencyAndBufferSize(72-75)
core/bifrost.go (2)
core/schemas/bifrost.go (3)
Ollama(46-46)ModelProvider(36-36)Vertex(44-44)core/providers/ollama.go (1)
NewOllamaProvider(59-87)
🪛 LanguageTool
README.md
[style] ~353-~353: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...cumentation improvements, or new ideas. Feel free to open an issue, and once it's assigned, ...
(FEEL_FREE_TO_STYLE_ME)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (12)
docs/providers.md (1)
15-16: Approve addition of Mistral and Ollama providers
The documentation now reflects the new providers supported by Bifrost.core/schemas/bifrost.go (1)
46-47: AddOllamatoModelProviderenumeration
This aligns with the new Ollama provider implementation.docs/http-transport-api.md (2)
230-233: Accept schema table update with new providers
Theproviderfield description now includesmistralandollamain the supported list.
454-456: Accept Supported Providers table expansion
The table now correctly listsmistralandollamaentries.docs/openapi.json (1)
435-444: Enum update looks correct.
mistralandollamaare now present and the spec stays in sync with the code.
No further action needed.README.md (1)
192-214: Docs refreshed correctly – LGTM.The overview & features sections now reference Mistral and Ollama and fix the “Built-in Observability” typo.
No issues spotted.core/tests/account.go (1)
30-39: Provider list updated – looks good.All currently supported providers, including
vertex,mistralandollama, are now returned.
No action required.core/bifrost.go (2)
143-145: Constructor wiring for Ollama propagates errors – good.The switch arm mirrors Bedrock/Azure style by returning both provider and error.
Looks correct.
354-358: Nice extraction ofproviderRequiresKey.Duplication removed and intent is clearer.
core/providers/ollama.go (3)
60-66:MaxConnsPerHostlikely set from the wrong knobEverywhere else the provider treats
Concurrencyas the limiting factor, yet thefasthttp.Clientis initialised withMaxConnsPerHost = BufferSize.
UnlessBufferSizeis deliberately distinct from concurrency, this mismatch will either under- or over-throttle the connection pool.- MaxConnsPerHost: config.ConcurrencyAndBufferSize.BufferSize, + MaxConnsPerHost: config.ConcurrencyAndBufferSize.Concurrency,Please double-check the intent and keep the two limits consistent.
68-72: Relying on “range over int” requires Go ≥ 1.22
for range config.ConcurrencyAndBufferSize.Concurrency { … }is great syntactic sugar, but it only compiles when the module’sgodirective is1.22or newer. Make surego.modalready declaresgo 1.22(or later) before this code lands.
130-136: Good call on conditionalAuthorizationheaderThe header is only set when a non-empty key is provided, keeping requests clean for the typical unauthenticated local Ollama setup.
Pratham-Mishra04
force-pushed
the
06-14-feat_ollama_provider_added
branch
from
511dcc0 to
3768593
Compare
Merge activity
|
akshaydeo
changed the base branch from
06-14-feat_mistral_provider_added
to
graphite-base/84
akshaydeo
force-pushed
the
06-14-feat_ollama_provider_added
branch
from
3768593 to
9456ec9
Compare
# Add Ollama Provider Support This PR adds support for the Ollama provider to Bifrost, expanding our multi-provider capabilities. Key changes include: - Implemented a new `OllamaProvider` that connects to local Ollama instances - Added provider-specific response handling and memory pooling for Ollama - Updated documentation to include Ollama in the list of supported providers - Added test coverage for the Ollama provider - Updated README to mention Mistral and Ollama in the list of supported providers The Ollama provider implementation follows our standard provider pattern but has some unique characteristics: - It doesn't require API keys like most other providers - It requires a base URL configuration to connect to the Ollama instance - It supports chat completions but not text completions This addition allows Bifrost users to leverage local LLM deployments through Ollama while maintaining the same consistent API interface used for cloud providers.
2 participants
Add Ollama Provider Support
This PR adds support for the Ollama provider to Bifrost, expanding our multi-provider capabilities. Key changes include:
OllamaProviderthat connects to local Ollama instancesThe Ollama provider implementation follows our standard provider pattern but has some unique characteristics:
This addition allows Bifrost users to leverage local LLM deployments through Ollama while maintaining the same consistent API interface used for cloud providers.