Skip to content

Conversation

@ArnabChatterjee20k
Copy link
Contributor

@ArnabChatterjee20k ArnabChatterjee20k commented Dec 2, 2025

Summary by CodeRabbit

  • Documentation

    • Standardized response field names across multiple AI provider integrations for improved consistency in metrics reporting and data structure.
  • Tests

    • Updated test cases to validate the new response field structure and metrics across all provider implementations.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

This pull request systematically updates the documented return shape of the embed() method across all embedding adapters and related test files. The docblock annotations are changed to replace total_duration and load_duration fields with tokensProcessed, totalDuration, and modelLoadingDuration. No changes are made to method signatures, runtime behavior, or control flow. Tests are updated to assert against the new field names.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

  • Repetitive pattern: The same docblock field renaming is applied consistently across 8 adapter files (Anthropic, Deepseek, Gemini, Ollama, OpenAI, Perplexity, XAI) plus the abstract base class and test file, reducing cognitive load
  • Homogeneous changes: All modifications follow an identical pattern (field name swaps only)
  • Documentation-only updates: No logic, control flow, or actual behavior changes
  • Areas requiring attention:
    • Verify consistency of field name mappings across all adapter implementations
    • Confirm test assertions in tests/Agents/AgentTest.php cover all three new fields (tokensProcessed, totalDuration, modelLoadingDuration)
    • Check that the Ollama adapter's note about totalDuration encompassing modelLoadingDuration is correctly documented and implemented

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Add tokens processed output from the embed method' accurately reflects the main change: adding a new tokensProcessed field to the embed method's return documentation across all adapter classes and tests.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat-embeddings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 nit

The 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 since getSupportForEmbeddings() is false).
tests/Agents/AgentTest.php (1)

107-112: Embedding tests correctly validate new metrics; be aware of coupling to Ollama timings

The extra checks for tokensProcessed, totalDuration, and modelLoadingDuration (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 0 or 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 consistent

The 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 in totalDuration)

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 to int|null, for consistency.

Also applies to: 88-94

src/Agents/Adapters/XAI.php (1)

171-173: XAI embed() docblock aligned with shared embedding contract

The 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 metrics

The abstract embed() return type now describes tokensProcessed, totalDuration, and modelLoadingDuration, which matches the concrete Ollama implementation and the updated tests.

Optional: normalize int|null , to int|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 shape

The 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 schema

The new embed() return annotation matches the standardized embedding schema used by Adapter and Agent, 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 to int|null, to match typical PHPDoc style.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74c0a70 and 6efdc62.

📒 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 good

The 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 shape

The 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 consistent

The 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 contract

The embed() docblock now documents embedding plus tokensProcessed, totalDuration, and modelLoadingDuration, matching the new standard return shape across adapters without altering behavior.

@abnegate abnegate merged commit ab026e4 into main Dec 2, 2025
4 checks passed
@abnegate abnegate deleted the feat-embeddings branch December 2, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants