Skip to content

fix: Simplifed langfuse auth check #177

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

Merged
merged 5 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions packages/exchange/.env.langfuse.local
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,3 @@ LANGFUSE_INIT_USER_PASSWORD=localpwd
LANGFUSE_INIT_ORG_ID=local-id
LANGFUSE_INIT_ORG_NAME=local-org
LANGFUSE_INIT_PROJECT_ID=goose

Copy link
Collaborator

Choose a reason for hiding this comment

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

for posterity: long-term we'll consider moving this to a app level config

# These variables are used by Goose
LANGFUSE_PUBLIC_KEY=publickey-local
LANGFUSE_SECRET_KEY=secretkey-local
LANGFUSE_HOST=http://localhost:3000
33 changes: 10 additions & 23 deletions packages/exchange/src/exchange/langfuse_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,29 +14,26 @@

import os
from typing import Callable
from dotenv import load_dotenv
from langfuse.decorators import langfuse_context
import sys
from io import StringIO
from pathlib import Path
from functools import wraps # Add this import
from functools import cache, wraps # Add this import
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: comment unnecessary



def find_package_root(start_path: Path, marker_file: str = "pyproject.toml") -> Path:
while start_path != start_path.parent:
if (start_path / marker_file).exists():
return start_path
start_path = start_path.parent
return None
DEFAULT_LOCAL_LANGFUSE_HOST = "http://localhost:3000"
DEFAULT_LOCAL_LANGFUSE_PUBLIC_KEY = "publickey-local"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe add a comment here that the public/private key defaults come from the .env.langfuse.local file. As these are what langfuse is locally initialized with as defined there. These values are not actually default from Langfuse itself.

DEFAULT_LOCAL_LANGFUSE_SECRET_KEY = "secretkey-local"


@cache
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice! this is a lot cleaner

Copy link
Collaborator

Choose a reason for hiding this comment

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

how come we're memoizing this? seems like a premature optimization?

the env check is cheap enough it shouldn't matter. now goose can't get new env variables when set externally (i.e. next tracing event) because there is no cache invalidation

Copy link
Collaborator Author

@lifeizhou-ap lifeizhou-ap Oct 22, 2024

Choose a reason for hiding this comment

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

Basically I did not change the existing behaviour. The purpose of the PR is mainly to remove the assumption of .env.langfuse.local file is part of exchange package, and rendering the local Langfuse credential from environment variables. (or it can be rendered from a environment file provided by goose application)

At the moment, I still leave the @cache method here as it is overkill to call the auth_check every time a function with observe_wrapper decorator is called.

By looking into the implementation, currently we rely on the auth_check to decide whether we want to trace the api call instead of deciding by whether the user passes --tracing option. I think the improvement for this part could be:

  • if goose session starts with --tracing, the application should raise with error message when error occurs with tracing
  • If goose session starts without --tracing, then we don't need to perform auth_check

def auth_check() -> bool:
# Temporarily redirect stdout and stderr to suppress print statements from Langfuse
temp_stderr = StringIO()
sys.stderr = temp_stderr

# Load environment variables
load_dotenv(LANGFUSE_ENV_FILE, override=True)
# Set environment variables if not specified
os.environ.setdefault("LANGFUSE_PUBLIC_KEY", DEFAULT_LOCAL_LANGFUSE_PUBLIC_KEY)
os.environ.setdefault("LANGFUSE_SECRET_KEY", DEFAULT_LOCAL_LANGFUSE_SECRET_KEY)
os.environ.setdefault("LANGFUSE_HOST", DEFAULT_LOCAL_LANGFUSE_HOST)

auth_val = langfuse_context.auth_check()

Expand All @@ -45,16 +42,6 @@ def auth_check() -> bool:
return auth_val


CURRENT_DIR = Path(__file__).parent
PACKAGE_ROOT = find_package_root(CURRENT_DIR)

LANGFUSE_ENV_FILE = os.path.join(PACKAGE_ROOT, ".env.langfuse.local") if PACKAGE_ROOT else None
Copy link
Collaborator

Choose a reason for hiding this comment

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

i didn't mind this approach, if feel what we could've improved here is deferred the loading so it wasn't during module init. moving everything to a environment variable just pollutes how we manage configs more

personally i feel approach here is to configure them in $XDG_CONFIG_HOME to centralize it.

HAS_LANGFUSE_CREDENTIALS = False
load_dotenv(LANGFUSE_ENV_FILE, override=True)

HAS_LANGFUSE_CREDENTIALS = auth_check()


def observe_wrapper(*args, **kwargs) -> Callable: # noqa
"""
A decorator that wraps a function with Langfuse context observation if credentials are available.
Expand All @@ -71,7 +58,7 @@ def observe_wrapper(*args, **kwargs) -> Callable: # noqa
"""

def _wrapper(fn: Callable) -> Callable:
if HAS_LANGFUSE_CREDENTIALS:
if auth_check():

@wraps(fn)
def wrapped_fn(*fargs, **fkwargs): # noqa
Expand Down
50 changes: 26 additions & 24 deletions packages/exchange/tests/test_langfuse_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,38 +9,40 @@ def mock_langfuse_context():
yield mock


@patch("exchange.langfuse_wrapper.HAS_LANGFUSE_CREDENTIALS", True)
def test_function_is_wrapped(mock_langfuse_context):
mock_observe = MagicMock(side_effect=lambda *args, **kwargs: lambda fn: fn)
mock_langfuse_context.observe = mock_observe
with patch("exchange.langfuse_wrapper.auth_check") as mocked_auth_check:
Copy link
Collaborator

@lamchau lamchau Oct 21, 2024

Choose a reason for hiding this comment

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

why did we go with this approach? i deliberately wrote the tests as a decorator to reduce how much nesting the testing code has

mocked_auth_check.return_value = True
mock_observe = MagicMock(side_effect=lambda *args, **kwargs: lambda fn: fn)
mock_langfuse_context.observe = mock_observe

def original_function(x: int, y: int) -> int:
return x + y
def original_function(x: int, y: int) -> int:
return x + y

# test function before we decorate it with
# @observe_wrapper("arg1", kwarg1="kwarg1")
assert not hasattr(original_function, "__wrapped__")
# test function before we decorate it with
# @observe_wrapper("arg1", kwarg1="kwarg1")
assert not hasattr(original_function, "__wrapped__")

# ensure we args get passed along (e.g. @observe(capture_input=False, capture_output=False))
decorated_function = observe_wrapper("arg1", kwarg1="kwarg1")(original_function)
assert hasattr(decorated_function, "__wrapped__")
assert decorated_function.__wrapped__ is original_function, "Function is not properly wrapped"
# ensure we args get passed along (e.g. @observe(capture_input=False, capture_output=False))
decorated_function = observe_wrapper("arg1", kwarg1="kwarg1")(original_function)
assert hasattr(decorated_function, "__wrapped__")
assert decorated_function.__wrapped__ is original_function, "Function is not properly wrapped"

assert decorated_function(2, 3) == 5
mock_observe.assert_called_once()
mock_observe.assert_called_with("arg1", kwarg1="kwarg1")
assert decorated_function(2, 3) == 5
mock_observe.assert_called_once()
mock_observe.assert_called_with("arg1", kwarg1="kwarg1")


@patch("exchange.langfuse_wrapper.HAS_LANGFUSE_CREDENTIALS", False)
def test_function_is_not_wrapped(mock_langfuse_context):
mock_observe = MagicMock(return_value=lambda f: f)
mock_langfuse_context.observe = mock_observe
with patch("exchange.langfuse_wrapper.auth_check") as mocked_auth_check:
mocked_auth_check.return_value = False
mock_observe = MagicMock(return_value=lambda f: f)
mock_langfuse_context.observe = mock_observe

@observe_wrapper("arg1", kwarg1="kwarg1")
def hello() -> str:
return "Hello"
@observe_wrapper("arg1", kwarg1="kwarg1")
def hello() -> str:
return "Hello"

assert not hasattr(hello, "__wrapped__")
assert hello() == "Hello"
assert not hasattr(hello, "__wrapped__")
assert hello() == "Hello"

mock_observe.assert_not_called()
mock_observe.assert_not_called()