fix: Simplifed langfuse auth check #177
Conversation
| start_path = start_path.parent | ||
| return None | ||
| DEFAULT_LOCAL_LANGFUSE_HOST = "http://localhost:3000" | ||
| DEFAULT_LOCAL_LANGFUSE_PUBLIC_KEY = "publickey-local" |
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| @cache |
There was a problem hiding this comment.
nice! this is a lot cleaner
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 performauth_check
| 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 |
There was a problem hiding this comment.
nit: comment unnecessary
| 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: |
There was a problem hiding this comment.
why did we go with this approach? i deliberately wrote the tests as a decorator to reduce how much nesting the testing code has
| 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 |
There was a problem hiding this comment.
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.
|
we shouldn't be introducing more environment variables rather we should be doing the opposite. if we really decide to keep going down this path we should at least namespace it (e.g. shell environments are a global shared state. anyhoo lgtm, just something we need to consider |
| @@ -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 | |||
|
|
|||
There was a problem hiding this comment.
for posterity: long-term we'll consider moving this to a app level config
What
Currently the env.langfuse.local is used for 2 purposes:
For item 2, it would be better to specify the values on the goose client side (user) instead of packaging into the exchange package, either via loading an environment file that user specifies the path or loading the environment variable that user sets.
What
In exchange, rendering the environment variables for langfuse host, public key, private key, if any of them does not exists, fallback to the default variable for localhost langfuse