Skip to content

fix(langchain): silence intentional placeholder warnings#1689

Open
fengjikui wants to merge 1 commit into
langfuse:mainfrom
fengjikui:fix-langchain-placeholder-warning
Open

fix(langchain): silence intentional placeholder warnings#1689
fengjikui wants to merge 1 commit into
langfuse:mainfrom
fengjikui:fix-langchain-placeholder-warning

Conversation

@fengjikui
Copy link
Copy Markdown

@fengjikui fengjikui commented Jun 7, 2026

Summary

  • Keep unresolved-placeholder warnings for direct ChatPromptClient.compile() calls.
  • Stop emitting that warning from get_langchain_prompt() when unresolved placeholders are intentionally returned as LangChain MessagesPlaceholder objects.
  • Add regression coverage to ensure the LangChain hand-off path stays quiet while preserving the existing placeholder output.

Fixes #1667.

Verification

  • uv run --frozen pytest tests/unit/test_prompt_compilation.py::TestLangchainPromptCompilation::test_get_langchain_prompt_with_unresolved_placeholders -q
  • uv run --frozen pytest tests/unit/test_prompt_compilation.py -q
  • uv run --frozen ruff check langfuse/model.py tests/unit/test_prompt_compilation.py
  • uv run --frozen ruff format --check langfuse/model.py tests/unit/test_prompt_compilation.py
  • uv run --frozen mypy langfuse --no-error-summary
  • git diff --check

I also tried uv run --frozen pytest tests/e2e/test_prompt.py::test_warning_on_unresolved_placeholders -q, but the local run requires Langfuse server credentials/API setup and failed before the assertion path with Langfuse client initialized without public_key.

Greptile Summary

This PR fixes spurious langfuse_logger.warning calls that fire during get_langchain_prompt() when unresolved placeholders are intentionally left for LangChain to handle as MessagesPlaceholder objects.

  • compile() is refactored into a thin wrapper around a new private _compile(warn_on_unresolved_placeholders: bool) method; the public API is unchanged and still warns by default.
  • get_langchain_prompt() now calls _compile(warn_on_unresolved_placeholders=False), suppressing the warning only on the LangChain hand-off path.
  • A regression test asserts that langfuse_logger.warning is never called during get_langchain_prompt() when a placeholder is intentionally left unresolved.

Confidence Score: 4/5

Safe to merge; the behavioural change is narrow and well-tested.

The refactor correctly isolates warning behaviour behind a flag, public compile() is unchanged, and a targeted regression test covers the silent path. The only finding is a style-level import placed inside a test method body rather than at the module top.

No files require special attention beyond the minor import placement in the test.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["compile(kwargs)"] -->|"warn=True"| B["_compile()"]
    C["get_langchain_prompt(kwargs)"] -->|"warn=False"| B

    B --> D{placeholder resolved?}
    D -- Yes --> E[expand messages]
    D -- No --> F[keep as placeholder dict]
    F --> G{warn flag set?}
    G -- Yes --> H["langfuse_logger.warning()"]
    G -- No --> I[silent]

    A --> L[return raw dicts]
    C --> M["convert to LangChain tuples / MessagesPlaceholder"]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
tests/unit/test_prompt_compilation.py:805-810
The `patch` import is placed inside the test method body. The project rule requires imports to be at the top of the module. The two pre-existing local imports (`Prompt_Chat`, `ChatPromptClient`) also violate this rule, but this PR actively adds a third one (`from unittest.mock import patch`) and is a good opportunity to move at least the new import to the top-level block.

```suggestion
    def test_get_langchain_prompt_with_unresolved_placeholders(self):
        """Test that unresolved placeholders become MessagesPlaceholder objects."""
        from langfuse.api import Prompt_Chat
        from langfuse.model import ChatPromptClient
```

Reviews (1): Last reviewed commit: "fix(langchain): silence intentional plac..." | Re-trigger Greptile

Context used:

  • Rule used - Move imports to the top of the module instead of p... (source)

Learned From
langfuse/langfuse-python#1387

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 7, 2026

CLA assistant check
All committers have signed the CLA.

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.

Suppress "Placeholders have not been resolved" warning when MessagesPlaceholder use is intentional (LangChain integration)

2 participants