Skip to content

Add tests for max_tokens propagation in LLM methods - #925

Open
saiganesh47 wants to merge 1 commit into
semantica-agi:mainfrom
saiganesh47:patch-2
Open

Add tests for max_tokens propagation in LLM methods#925
saiganesh47 wants to merge 1 commit into
semantica-agi:mainfrom
saiganesh47:patch-2

Conversation

@saiganesh47

Copy link
Copy Markdown

This test verifies that the max_tokens parameter is correctly propagated to the generate_typed method for different extraction functions.

Before you submit: make sure you followed the issue workflow in CONTRIBUTING.md — comment on the issue and wait for assignment before opening a PR, to avoid duplicate work.

Description

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code refactoring

Related Issues

Closes #
Fixes #

Changes Made

Testing

  • Tested locally
  • Added tests for new functionality
  • Package builds successfully (python -m build)

Test Commands

# Build the package
pip install build
python -m build

# Optional: Run your own tests
pytest tests/

# Optional: Format code
black semantica/
isort semantica/

Documentation

  • Updated relevant documentation
  • Added code examples if applicable
  • Updated API reference if adding new APIs
  • Updated cookbook if adding new examples
  • No documentation changes needed

Breaking Changes

Breaking Changes: [Yes/No]

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Package builds successfully

Additional Notes

This test verifies that the max_tokens parameter is correctly propagated to the generate_typed method for different extraction functions.
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Add unit tests ensuring max_tokens is forwarded to LLM generate_typed()

🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Add unit tests covering max_tokens propagation across entity, relation, and triplet extraction
 helpers.
• Mock provider creation to assert generate_typed() receives the configured max_tokens value.
Diagram

graph TD
  T["TestMaxTokensPropagation"] --> M["extract_*_llm"] --> P["create_provider()"] --> L["LLM provider"] --> G["generate_typed()"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Parametrize with pytest
  • ➕ Less repetition: one parametrized test can cover all extraction functions
  • ➕ Cleaner assertion reporting per parameter set
  • ➖ Requires pytest adoption/usage patterns if the repo primarily uses unittest
  • ➖ May need minor test harness changes (fixtures, markers)
2. Assert full call signature for generate_typed()
  • ➕ Catches accidental changes to other forwarded kwargs beyond max_tokens
  • ➖ More brittle tests (fails on unrelated internal refactors)
  • ➖ Harder to maintain across provider implementations

Recommendation: Current approach (unittest + shared helper method + patched create_provider) is a good balance of readability and stability for verifying max_tokens forwarding. If the project standardizes on pytest, consider converting this to a single parametrized test to reduce duplication, but it’s not required for correctness.

Files changed (1) +93 / -0

Tests (1) +93 / -0
optimize reproduce_issue_176.pyAdd regression tests for max_tokens propagation into generate_typed() +93/-0

Add regression tests for max_tokens propagation into generate_typed()

• Introduces a unittest.TestCase that patches create_provider to return a mocked LLM provider. Adds three tests (entities, relations, triplets) that call the public extraction helpers with max_tokens and assert generate_typed() receives the same value.

tests/optimize reproduce_issue_176.py

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Test not collected 🐞 Bug ☼ Reliability
Description
The added file tests/optimize reproduce_issue_176.py is unlikely to be collected by pytest because
it doesn’t match default test_*.py / *_test.py patterns and includes a space in the filename. As
a result, CI likely won’t execute these tests, so the PR won’t actually add coverage.
Code

tests/optimize reproduce_issue_176.py[R1-4]

+import unittest
+from unittest.mock import MagicMock, patch
+
+from semantica.semantic_extract.methods import (
Evidence
Pytest is configured with testpaths = ["tests"] but no python_files override, so only default
patterns are collected; the added file’s name does not match those patterns and contains a space.

pyproject.toml[274-276]
tests/optimize reproduce_issue_176.py[1-4]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test module name (`optimize reproduce_issue_176.py`) won’t be discovered by pytest’s default collection rules and the space in the filename makes it awkward/brittle to run from shells/CI.

## Issue Context
`pyproject.toml` configures `testpaths` but does not override `python_files`, so pytest uses its default module patterns.

## Fix Focus Areas
- pyproject.toml[274-276]
- tests/optimize reproduce_issue_176.py[1-93]

## Suggested fix
- Rename/move the file to a pytest-discoverable name, e.g. `tests/semantic_extract/test_max_tokens_propagation.py` (or `tests/test_max_tokens_propagation.py`).
- Avoid spaces in filenames.
- (Optional) If you truly want nonstandard names, explicitly set `[tool.pytest.ini_options].python_files` in `pyproject.toml`, but renaming is preferred.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Relations max_tokens dropped 🐞 Bug ≡ Correctness
Description
The new test asserts extract_relations_llm() forwards max_tokens into llm.generate_typed(),
but extract_relations_llm() currently builds call_kwargs without max_tokens before calling
generate_typed. Once the test is made discoverable, it will fail unless production code is updated
(or the assertion is changed).
Code

tests/optimize reproduce_issue_176.py[R45-48]

+        self.assertEqual(
+            call_kwargs.get("max_tokens"),
+            self.MAX_TOKENS,
+            "max_tokens was not propagated correctly",
Evidence
The test passes max_tokens and asserts it appears in the generate_typed kwargs, but the
production implementation for relations explicitly constructs call_kwargs without max_tokens and
passes only that dict into generate_typed.

tests/optimize reproduce_issue_176.py[33-49]
semantica/semantic_extract/methods.py[1904-1916]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`extract_relations_llm()` does not propagate `max_tokens` to `llm.generate_typed()`, unlike `extract_entities_llm()` / `extract_triplets_llm()`. The newly added test expects `max_tokens` to be present in the `generate_typed` call kwargs.

## Issue Context
In `extract_relations_llm()`, the code constructs a limited `call_kwargs` dict (temperature/verbose/max_retries) and calls `llm.generate_typed(..., **call_kwargs)`, which drops `max_tokens`.

## Fix Focus Areas
- semantica/semantic_extract/methods.py[1904-1916]
- tests/optimize reproduce_issue_176.py[33-49]

## Suggested fix
- Include `max_tokens` (and any other intended generation limit knobs) in `call_kwargs` when present in `kwargs`, e.g.:
 - `if "max_tokens" in kwargs: call_kwargs["max_tokens"] = kwargs["max_tokens"]`
- Ensure the updated behavior is consistent with `extract_entities_llm()` / `extract_triplets_llm()` so the test can pass once properly collected.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Duplicate issue-176 test script 🐞 Bug ⚙ Maintainability
Description
The PR adds another issue-176 max_tokens propagation test script, but the repository already
contains tests/reproduce_issue_176.py covering the same three methods. Keeping multiple
near-duplicates makes it unclear which one is canonical and increases the chance of inconsistent
future edits.
Code

tests/optimize reproduce_issue_176.py[R12-18]

+class TestMaxTokensPropagation(unittest.TestCase):
+    MAX_TOKENS = 128000
+
+    def setUp(self):
+        self.mock_llm = MagicMock()
+        self.mock_llm.is_available.return_value = True
+
Evidence
The newly added file defines the same TestMaxTokensPropagation suite already present in
tests/reproduce_issue_176.py (relations/entities/triplets propagation checks).

tests/optimize reproduce_issue_176.py[12-90]
tests/reproduce_issue_176.py[1-98]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
There are now two scripts testing the same max_tokens propagation scenario (`tests/reproduce_issue_176.py` and the newly added `tests/optimize reproduce_issue_176.py`). This duplication is confusing and makes it easy for one to drift.

## Issue Context
The new file appears to be a cleaner refactor of the existing reproduce script.

## Fix Focus Areas
- tests/optimize reproduce_issue_176.py[1-93]
- tests/reproduce_issue_176.py[1-100]

## Suggested fix
- Choose a single canonical test module (preferably pytest-discoverable, e.g. `tests/semantic_extract/test_max_tokens_propagation.py`).
- Move/rename the better version there.
- Delete the redundant reproduce script(s) or keep *one* reproduction script outside `tests/` if it’s meant for manual runs only.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +1 to +4
import unittest
from unittest.mock import MagicMock, patch

from semantica.semantic_extract.methods import (

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Test not collected 🐞 Bug ☼ Reliability

The added file tests/optimize reproduce_issue_176.py is unlikely to be collected by pytest because
it doesn’t match default test_*.py / *_test.py patterns and includes a space in the filename. As
a result, CI likely won’t execute these tests, so the PR won’t actually add coverage.
Agent Prompt
## Issue description
The new test module name (`optimize reproduce_issue_176.py`) won’t be discovered by pytest’s default collection rules and the space in the filename makes it awkward/brittle to run from shells/CI.

## Issue Context
`pyproject.toml` configures `testpaths` but does not override `python_files`, so pytest uses its default module patterns.

## Fix Focus Areas
- pyproject.toml[274-276]
- tests/optimize reproduce_issue_176.py[1-93]

## Suggested fix
- Rename/move the file to a pytest-discoverable name, e.g. `tests/semantic_extract/test_max_tokens_propagation.py` (or `tests/test_max_tokens_propagation.py`).
- Avoid spaces in filenames.
- (Optional) If you truly want nonstandard names, explicitly set `[tool.pytest.ini_options].python_files` in `pyproject.toml`, but renaming is preferred.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +45 to +48
self.assertEqual(
call_kwargs.get("max_tokens"),
self.MAX_TOKENS,
"max_tokens was not propagated correctly",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Relations max_tokens dropped 🐞 Bug ≡ Correctness

The new test asserts extract_relations_llm() forwards max_tokens into llm.generate_typed(),
but extract_relations_llm() currently builds call_kwargs without max_tokens before calling
generate_typed. Once the test is made discoverable, it will fail unless production code is updated
(or the assertion is changed).
Agent Prompt
## Issue description
`extract_relations_llm()` does not propagate `max_tokens` to `llm.generate_typed()`, unlike `extract_entities_llm()` / `extract_triplets_llm()`. The newly added test expects `max_tokens` to be present in the `generate_typed` call kwargs.

## Issue Context
In `extract_relations_llm()`, the code constructs a limited `call_kwargs` dict (temperature/verbose/max_retries) and calls `llm.generate_typed(..., **call_kwargs)`, which drops `max_tokens`.

## Fix Focus Areas
- semantica/semantic_extract/methods.py[1904-1916]
- tests/optimize reproduce_issue_176.py[33-49]

## Suggested fix
- Include `max_tokens` (and any other intended generation limit knobs) in `call_kwargs` when present in `kwargs`, e.g.:
  - `if "max_tokens" in kwargs: call_kwargs["max_tokens"] = kwargs["max_tokens"]`
- Ensure the updated behavior is consistent with `extract_entities_llm()` / `extract_triplets_llm()` so the test can pass once properly collected.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +12 to +18
class TestMaxTokensPropagation(unittest.TestCase):
MAX_TOKENS = 128000

def setUp(self):
self.mock_llm = MagicMock()
self.mock_llm.is_available.return_value = True

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

3. Duplicate issue-176 test script 🐞 Bug ⚙ Maintainability

The PR adds another issue-176 max_tokens propagation test script, but the repository already
contains tests/reproduce_issue_176.py covering the same three methods. Keeping multiple
near-duplicates makes it unclear which one is canonical and increases the chance of inconsistent
future edits.
Agent Prompt
## Issue description
There are now two scripts testing the same max_tokens propagation scenario (`tests/reproduce_issue_176.py` and the newly added `tests/optimize reproduce_issue_176.py`). This duplication is confusing and makes it easy for one to drift.

## Issue Context
The new file appears to be a cleaner refactor of the existing reproduce script.

## Fix Focus Areas
- tests/optimize reproduce_issue_176.py[1-93]
- tests/reproduce_issue_176.py[1-100]

## Suggested fix
- Choose a single canonical test module (preferably pytest-discoverable, e.g. `tests/semantic_extract/test_max_tokens_propagation.py`).
- Move/rename the better version there.
- Delete the redundant reproduce script(s) or keep *one* reproduction script outside `tests/` if it’s meant for manual runs only.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@Sameer6305

Copy link
Copy Markdown
Collaborator

@saiganesh47 can you please handle qodo findings, so we can proceed for the review.

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.

2 participants