Skip to content

fix(core): make yield_keys prefix keyword-only to match BaseStore#34659

Merged
Mason Daugherty (mdrxy) merged 5 commits into
langchain-ai:masterfrom
Krud-x:fix-keyword-only-prefix
Jan 10, 2026
Merged

fix(core): make yield_keys prefix keyword-only to match BaseStore#34659
Mason Daugherty (mdrxy) merged 5 commits into
langchain-ai:masterfrom
Krud-x:fix-keyword-only-prefix

Conversation

@Krud-x
Copy link
Copy Markdown
Contributor

@Krud-x Krud-x commented Jan 8, 2026

This PR fixes a signature mismatch between BaseStore and its concrete
implementations by making the prefix parameter keyword-only in
yield_keys and ayield_keys.

This aligns the implementations with the BaseStore interface contract,
prevents Liskov Substitution Principle violations, and ensures consistent
method signatures across store backends.

Fixes #32637

Breaking changes
None. This change only enforces the existing abstract interface and does
not modify runtime behavior

Testing

  • Verified that existing test suites pass after the signature fix.

Parts of this contribution were assisted by generative AI for
code navigation and drafting. All final design decisions and changes were
reviewed and validated manually.

@github-actions github-actions Bot added core `langchain-core` package issues & PRs langchain-classic `langchain-classic` package issues & PRs fix For PRs that implement a fix labels Jan 8, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Jan 8, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 13 untouched benchmarks
⏩ 21 skipped benchmarks1


Comparing Krud-x:fix-keyword-only-prefix (6eca77c) with master (c080296)

Open in CodSpeed

Footnotes

  1. 21 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Krud-x Krud-x changed the title fix(stores): make yield_keys prefix keyword-only to match BaseStore fix(core): make yield_keys prefix keyword-only to match BaseStore Jan 8, 2026
@github-actions github-actions Bot added fix For PRs that implement a fix and removed fix For PRs that implement a fix labels Jan 8, 2026
@mdrxy Mason Daugherty (mdrxy) changed the title fix(core): make yield_keys prefix keyword-only to match BaseStore fix(core): make yield_keys prefix keyword-only to match BaseStore Jan 8, 2026
@github-actions github-actions Bot added fix For PRs that implement a fix and removed fix For PRs that implement a fix labels Jan 8, 2026
@mdrxy
Copy link
Copy Markdown
Member

Krud-x could you resolve CI failures please?

@mdrxy Mason Daugherty (mdrxy) added the waiting-on-author Issue or PR is paused, waiting to hear from author in order to proceed label Jan 8, 2026
@aaron-seq
Copy link
Copy Markdown
Contributor

Code Review

Overview

This PR correctly addresses the signature mismatch between BaseStore and its concrete implementations by making the prefix parameter keyword-only in yield_keys() and ayield_keys() methods. This is a good fix that enforces the Liskov Substitution Principle and aligns with the abstract interface contract.

CI/CD Failure Analysis

The PR is currently failing 11 CI/CD checks due to test code that needs to be updated. The failures are caused by existing tests that call yield_keys() with positional arguments, which is no longer allowed after your changes.

Root Cause

The mypy type checker is correctly identifying violations in tests/unit_tests/storage/test_filesystem.py at lines:

  • Line 143: Too many positional arguments for "yield_keys" of "LocalFileStore"
  • Line 149: Too many positional arguments for "yield_keys" of "LocalFileStore"
  • Line 158: Too many positional arguments for "yield_keys" of "LocalFileStore"

These test files are calling yield_keys(prefix_value) with a positional argument, but after adding *, in the signature, the prefix parameter must be passed as a keyword argument: yield_keys(prefix=prefix_value).

Required Fix

You need to update the test file tests/unit_tests/storage/test_filesystem.py to use keyword arguments. The calls should be changed from:

# Current (incorrect after your changes)
store.yield_keys(some_prefix)

To:

# Correct
store.yield_keys(prefix=some_prefix)

Code Logic Assessment

The core code changes are correct:

  1. Signature changes in libs/core/langchain_core/stores.py:

    • Adding *, before prefix in both yield_keys() and ayield_keys() correctly makes the parameter keyword-only
    • This matches the BaseStore abstract interface
  2. Signature changes in libs/langchain/langchain_classic/storage/file_system.py:

    • Correctly implements the same keyword-only pattern in the concrete implementation
    • Maintains consistency with the base class

Additional Considerations

  1. No Breaking Changes to Runtime Behavior: As you correctly noted, this change only enforces the existing interface contract and doesn't modify runtime behavior.

  2. Other Potential Call Sites: Beyond the test file, there may be other places in the codebase (examples, documentation, or other implementations) that call yield_keys() with positional arguments. The failing test jobs suggest there might be more instances to fix.

  3. Search for All Occurrences: Run a codebase-wide search for yield_keys( and ayield_keys( patterns to ensure all call sites are updated to use keyword arguments.

Recommendation

Update tests/unit_tests/storage/test_filesystem.py at lines 143, 149, and 158 to use keyword arguments for the prefix parameter. After fixing these test calls, the CI should pass.

References

@Krud-x
Copy link
Copy Markdown
Contributor Author

Krud-x commented Jan 9, 2026

I’ve updated the failing tests to use keyword-only arguments for yield_keys and ayield_keys as suggested.
CI should now reflect the fix. Thanks for the review!

Comment thread test.sh Outdated
@mdrxy Mason Daugherty (mdrxy) removed the waiting-on-author Issue or PR is paused, waiting to hear from author in order to proceed label Jan 10, 2026
@mdrxy Mason Daugherty (mdrxy) merged commit d22cfaf into langchain-ai:master Jan 10, 2026
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core `langchain-core` package issues & PRs external fix For PRs that implement a fix langchain-classic `langchain-classic` package issues & PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChatPromptTemplate save() not implemented

4 participants