Feat/ollama cloud integration#890
Conversation
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for the PR! A few things before we can take another look:
pnpm-lock.yamlshouldn't be committed — this repo uses bun. Please remove it and keep the bun lockfile as the source of truth.- The
python/ollama_provider.pyandpython/smart_router.pyfiles look unrelated to this integration (no TS code references them). Could you drop them, or explain what they're for? - The branch currently has merge conflicts with main — please rebase.
Once those are cleaned up the scope becomes much easier to review. Appreciate the work on getting cloud support in.
f8b1488 to
04ac279
Compare
|
this will likely be impacted by #910 i think |
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for addressing the prior feedback — python files cleanup and Ollama handling fixes look good. Tested locally on commit 04ac279:
- Build: passes
- Test suite: 1596 pass, 5 fail. Only one failure is caused by this PR — the rest reproduce on main.
The PR-caused failure (src/tests/bugfixes.test.ts > 'Gemini store field fix > isGeminiMode is imported and used in openaiShim'):
The body.store comment block in openaiShim.ts was rewritten and no longer contains the substring "mistral and gemini don't recognize body.store" that the existing test asserts on. Functional behavior is fine (the guard still includes Mistral and Gemini), but the comment-text assertion now breaks.
Two ways to fix, either is fine:
- Restore the original phrasing somewhere in the comment so the .toContain check passes.
- Update the test to match the new comment (e.g. assert on
don't recognize body.storewithout the "mistral and gemini" prefix).
No red-flag rule hits — OLLAMA_API_KEY is a clean addition, nothing tengu/USER_TYPE/Anthropic-fingerprint-related. Once the test is green again I'm happy to approve.
gnanam1990
left a comment
There was a problem hiding this comment.
Thanks for the quick fix! Pulled the branch:
bun test src/__tests__/bugfixes.test.ts→ 26/26 pass- Full suite: 1713 pass, 2 fail — both failures reproduce on
main(detectProvider — modelOverride from --model flag), unrelated to this PR - Build clean, no openclaude red flags (no
tengu_*, no Anthropic fingerprints,OLLAMA_API_KEYis a clean env-only addition) - Main merged in cleanly
The remaining functional assertions in that test (isGeminiMode() usage + the isMistral || isGeminiMode() guard regex) still cover the real behavior, so dropping the comment-text assertion is fine.
LGTM 🚀
|
#910, please rebase and fix conflicts. providers/gateways have changed. |
ab9c06c to
92e16f0
Compare
|
I would request that this be rescoped considerably narrower. This PR is mostly to add a new provider, but feature creeps exponentially to changing many parts of the framework and how it functions. While im not against updating or improving the framework, it shouldn't be bundled with a provider addition. |
Summary
ollama.com/v1endpointOLLAMA_API_KEYauthentication for cloud-hosted modelsImpact
User-facing impact:
OLLAMA_API_KEYis setDeveloper/maintainer impact:
ollama-cloudprovider profile type and environment handlingTesting
bun run buildbun run smoke--provider ollama-cloud --api-key <key>Notes
cost_per_1k_tokens) marked as TBD pending Ollama's pricing structureisLikelyOllamaEndpoint()now recognizes ollama.com domains