-
Notifications
You must be signed in to change notification settings - Fork 0
Add tokens processed output from the embed method #17
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
…f tokens processed
WalkthroughThis pull request systematically updates the documented return shape of the Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Agents/Adapters/Deepseek.php (1)
326-333: Docblock return shape matches Adapter contract; minor docblock nitThe updated return type (embedding + tokensProcessed/totalDuration/modelLoadingDuration) is consistent with
Adapter::embed()and the rest of the PR.If you touch this again, consider:
- Fixing the small typo
int|null ,→int|null,for cleanliness.- Optionally clarifying in the PHPDoc that this adapter always throws for
embed()despite the return type (purely docs, behavior is fine sincegetSupportForEmbeddings()isfalse).tests/Agents/AgentTest.php (1)
107-112: Embedding tests correctly validate new metrics; be aware of coupling to Ollama timingsThe extra checks for
tokensProcessed,totalDuration, andmodelLoadingDuration(presence +> 0) are a good way to lock in the new shape.Just be aware this makes the tests slightly coupled to Ollama always returning positive values for these fields; if the API ever legitimately returns
0or omits a timing, these tests will start failing. That’s probably acceptable here, but worth keeping in mind.src/Agents/Adapters/Ollama.php (1)
55-59: Ollama embed() mapping to new metric fields is consistentThe concrete return payload correctly aligns with the new documented shape:
embedding←$json['embeddings'][0]tokensProcessed←$json['prompt_eval_count']totalDuration←$json['total_duration']modelLoadingDuration←$json['load_duration'](and the comment notes it’s included intotalDuration)This matches the updated Adapter/Agent contracts and the tests.
Minor nit only: the PHPDoc has
int|null ,— if you revisit this file, you may want to tidy that toint|null,for consistency.Also applies to: 88-94
src/Agents/Adapters/XAI.php (1)
171-173: XAI embed() docblock aligned with shared embedding contractThe updated return annotation for
embed()matches the shared Adapter/Agent embedding shape (embedding+ the three metric fields), which is helpful for consistency even though this adapter does not support embeddings.As elsewhere, you might optionally clean
int|null ,→int|null,when convenient.src/Agents/Adapter.php (1)
109-111: Central embed() contract updated correctly for new metricsThe abstract
embed()return type now describestokensProcessed,totalDuration, andmodelLoadingDuration, which matches the concrete Ollama implementation and the updated tests.Optional: normalize
int|null ,toint|null,here (and in implementations) for a slightly cleaner PHPDoc.src/Agents/Adapters/OpenAI.php (1)
427-429: OpenAI embed() PHPDoc kept in sync with shared embedding shapeThe updated return annotation for
embed()is consistent with the abstract Adapter contract and Agent’s embed() docs, even though this adapter doesn’t currently support embeddings.Again, the
int|null ,spacing is a tiny docblock nit you can optionally fix later.src/Agents/Adapters/Anthropic.php (1)
470-472: Anthropic embed() PHPDoc consistent with shared embedding schemaThe new
embed()return annotation matches the standardized embedding schema used byAdapterandAgent, which is good for consistency even though this adapter currently doesn’t implement embeddings.If desired, you can also clean up the
int|null ,spacing toint|null,to match typical PHPDoc style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Agents/Adapter.php(1 hunks)src/Agents/Adapters/Anthropic.php(1 hunks)src/Agents/Adapters/Deepseek.php(1 hunks)src/Agents/Adapters/Gemini.php(1 hunks)src/Agents/Adapters/Ollama.php(2 hunks)src/Agents/Adapters/OpenAI.php(1 hunks)src/Agents/Adapters/Perplexity.php(1 hunks)src/Agents/Adapters/XAI.php(1 hunks)src/Agents/Agent.php(1 hunks)tests/Agents/AgentTest.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (4)
tests/Agents/AgentTest.php (1)
132-137: Repeated metric assertions across varied inputs look goodThe looped assertions for the new metrics across different input lengths nicely exercise the embedding path and keep the expectations in sync with the Ollama adapter.
src/Agents/Agent.php (1)
146-148: Agent::embed() docblock now accurately documents embedding result shapeThe updated return annotation (embedding + tokensProcessed/totalDuration/modelLoadingDuration) matches the Adapter contract and the concrete Ollama implementation that powers the embedding tests, so callers now have a clear, consistent expectation for the embedding payload.
src/Agents/Adapters/Perplexity.php (1)
218-224: Docblock return shape update looks consistentThe updated return array keys (embedding, tokensProcessed, totalDuration, modelLoadingDuration) align with the new embed() contract across adapters. No functional impact and keeps Perplexity’s embed() documentation in sync with the rest of the codebase.
src/Agents/Adapters/Gemini.php (1)
358-364: Embed docblock aligned with unified metrics contractThe embed() docblock now documents embedding plus tokensProcessed, totalDuration, and modelLoadingDuration, matching the new standard return shape across adapters without altering behavior.
Summary by CodeRabbit
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.