Feature/posthog llm analytics#5
Conversation
- Add CallbackManager class for managing LLM analytics callbacks - Integrate PostHog analytics into PlotAgent with optional configuration - Add environment variable support for PostHog configuration - Add comprehensive tests for PostHog integration - Update pyproject.toml with optional posthog dependency - Add PostHog configuration to .env.example The analytics integration is disabled by default and can be enabled via POSTHOG_LLM_ANALYTICS environment variable. Supports both direct API key configuration and pre-configured PostHog client instances.
- Create plot_agent/constants.py with all default values and configuration - Extract hardcoded strings and defaults from agent.py, execution.py, callbacks.py - Add comprehensive constants for: - Agent configuration defaults (model, verbose, max_iterations, etc.) - LLM configuration (temperature, timeout, retries) - Execution environment settings (timeout, allowed modules) - PostHog configuration and environment variables - Error messages and response constants - Tool names and descriptions - Update all modules to use centralized constants - Fix test assertion to match new error message format - Export key constants in __init__.py for public API This improves maintainability by centralizing all configuration values and makes the codebase more consistent and easier to modify.
- Remove requirements.txt and requirements-dev.txt files - Add uv.lock for dependency management with uv - Update Makefile to use uv commands instead of pip - Maintain same dependency versions and functionality This migration provides faster dependency resolution and better reproducible builds using the modern uv package manager.
- Update README.md with latest features and usage instructions - Refresh all example notebooks with current output and results - Update run_log.txt with latest example execution results - Improve documentation clarity and add new feature descriptions These updates ensure documentation stays current with the latest codebase changes and provides users with accurate examples.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces PostHog analytics integration and centralized constants; refactors PlotAgent initialization and execution to use a callback manager and standardized messages; exposes package public API; migrates dependency and tooling to uv; updates CI, Makefile, and docs; raises Python requirement; adds tests and a pytest conftest for env isolation; refreshes example logs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant PA as PlotAgent
participant CM as CallbackManager
participant PH as PostHog (optional)
participant LLM as LLM/LangGraph
participant EX as Execution Env
rect rgb(245,248,255)
note over U,PA: Initialization
U->>PA: new PlotAgent(posthog_* args)
PA->>CM: create_callback_manager(...)
alt PostHog enabled and available
CM->>PH: init client/handler
CM-->>PA: callbacks configured
else
CM-->>PA: no callbacks
end
end
rect rgb(245,255,245)
note over U,PA: Message processing
U->>PA: process_message(message)
PA->>PA: validate df/message
PA->>LLM: invoke (with callbacks if any)
LLM-->>PA: response (may include code)
PA->>EX: execute extracted code (guarded)
EX-->>PA: result (success/error, fig/title/summary)
alt No figure produced
note right of PA: Guided retry
PA->>LLM: retry with guidance (reduced recursion)
LLM-->>PA: response (optional code)
PA->>EX: execute (if code present)
EX-->>PA: result
end
PA-->>U: final status/message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Updated all example notebooks with latest changes - Modified GitHub workflow configuration - Updated run log with latest execution results
- Changed from 'uv sync --dev' to 'uv sync --extra dev' - This ensures pytest and other dev dependencies are properly installed - Fixes the 'pytest not found' error in GitHub Actions
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces optional PostHog LLM analytics integration, improves developer experience with new Makefile targets and documentation, and refactors the PlotAgent class for better configuration and error handling.
- Adds optional PostHog LLM analytics configuration with new callback management system
- Improves developer workflow with new
make installtarget anduvdependency management - Refactors
PlotAgentto use constants for better maintainability and error handling
Reviewed Changes
Copilot reviewed 15 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plot_agent/constants.py | New constants file centralizing default values, error messages, and configuration |
| plot_agent/callbacks.py | New callback manager for PostHog LLM analytics integration |
| plot_agent/agent.py | Refactored to use constants and added PostHog configuration parameters |
| plot_agent/execution.py | Updated to use constants from centralized constants file |
| pyproject.toml | Added dependencies and optional extras for dev and posthog |
| Makefile | Added install target and switched to uv for dependency management |
| README.md | Enhanced with developer setup instructions and PostHog documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for var in REQUIRED_EXECUTION_VARIABLES: | ||
| if locals().get(var) is None: |
There was a problem hiding this comment.
Using locals() in this context will not access the variables from the executed code. The locals() call refers to the local scope of the execute_code method, not the namespace where the generated code was executed. This should check the execution namespace variables (fig, plot_title, plot_summary) directly as in the original code.
| for var in REQUIRED_EXECUTION_VARIABLES: | |
| if locals().get(var) is None: | |
| extracted_vars = {"fig": fig, "plot_title": plot_title, "plot_summary": plot_summary} | |
| for var in REQUIRED_EXECUTION_VARIABLES: | |
| if extracted_vars.get(var) is None: |
|
|
||
|
|
||
| def test_posthog_custom_host(monkeypatch): | ||
| """A custom PostHog host should be passed through when a handler is available.""" |
There was a problem hiding this comment.
Line 81 has trailing whitespace that should be removed for consistency.
| """A custom PostHog host should be passed through when a handler is available.""" | |
| """A custom PostHog host should be passed through when a handler is available.""" |
| monkeypatch.setenv("POSTHOG_PUBLIC_KEY", "phc_test_key") | ||
| monkeypatch.setenv("POSTHOG_HOST", "https://example.com") | ||
| monkeypatch.setattr( | ||
| "plot_agent.callbacks._load_posthog_handler", lambda: (DummyClient, DummyHandler) |
There was a problem hiding this comment.
Line 99 has trailing whitespace that should be removed for consistency.
| "plot_agent.callbacks._load_posthog_handler", lambda: (DummyClient, DummyHandler) | |
| "plot_agent.callbacks._load_posthog_handler", lambda: (DummyClient, DummyHandler) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
152-156: Update README Python requirement to >=3.9README.md (lines 152–156) currently says "Python 3.8 or higher"; change to "Python 3.9 or higher" to match pyproject.toml (requires-python = ">=3.9").
🧹 Nitpick comments (29)
examples/run_log.txt (5)
1-9: Don’t commit ephemeral run logs; move to artifacts and ignore in git.This file is environment- and date-dependent and will churn every run, creating noisy diffs. Recommend: publish as a CI artifact (or PostHog event), keep a single sample log in docs if needed, and add an ignore rule.
Add a gitignore rule (either file-specific or pattern-based):
+.gitignore +examples/run_log.txt +examples/*.logOptionally, keep a stable, redacted “sample” log under docs/ with fixed timestamps to illustrate format.
1-9: Timestamps lack timezone/offset; prefer ISO‑8601 with offset and run metadata.Use ISO 8601 with explicit offset (e.g., 2025-09-19T12:41:01.395Z or with ±hh:mm) and include commit SHA, python version, OS, CPU/GPU, and concurrency settings in the header to make runs comparable.
10-41: Interleaved concurrency logs are hard to follow; add worker/thread id or switch to structured logs.When multiple notebooks run concurrently, add a per-notebook job id and worker label to each line, or emit JSONL with fields {ts, level, job_id, notebook, event, duration}. This makes correlation and parsing resilient.
44-49: Execution summary: also log wall‑clock elapsed and concurrency to avoid confusion.Report both aggregate compute (sum of durations) and wall‑clock elapsed with the effective parallelism (e.g., N workers), so readers don’t misinterpret the totals.
1-53: Minor formatting nits.There are bare “timestamp - ” lines (Lines 1, 43, 50) that carry no message; consider removing or emitting an explicit marker to reduce noise.
.env.example (2)
10-13: Include full PostHog key options in the sample (commented) for clarity.The code/tests reference POSTHOG_PROJECT_API_KEY and POSTHOG_API_KEY. Add commented placeholders so users know what’s supported.
# Optional PostHog LLM analytics POSTHOG_LLM_ANALYTICS=false POSTHOG_PUBLIC_KEY="phc_your-public-key" POSTHOG_HOST="https://us.i.posthog.com" +# Optional: server-side keys (use one depending on your handler setup) +# POSTHOG_PROJECT_API_KEY=your-project-api-key +# POSTHOG_API_KEY=your-api-key
5-5: Nit: remove quotes to appease dotenv linters (values don’t need quoting).Keeps the example tidy and avoids linter warnings; functional behavior is unchanged.
-LANGSMITH_TRACING=false -LANGSMITH_ENDPOINT="https://api.smith.langchain.com" -LANGSMITH_API_KEY="your-api-key-here" -LANGSMITH_PROJECT="your-project-name-here" +LANGSMITH_TRACING=false +LANGSMITH_ENDPOINT=https://api.smith.langchain.com +LANGSMITH_API_KEY=your-api-key-here +LANGSMITH_PROJECT=your-project-name-here POSTHOG_LLM_ANALYTICS=false -POSTHOG_PUBLIC_KEY="phc_your-public-key" -POSTHOG_HOST="https://us.i.posthog.com" +POSTHOG_PUBLIC_KEY=phc_your-public-key +POSTHOG_HOST=https://us.i.posthog.comAlso applies to: 12-13
.github/workflows/test.yml (1)
34-34: Add a trailing newline to satisfy linters.- uv run pytest tests/ -v + uv run pytest tests/ -v +pyproject.toml (2)
14-26: Drop redundant dependency on langchain-core.langchain already depends on langchain-core; keeping both can complicate resolution.
dependencies = [ "pandas", "numpy", "matplotlib", "plotly", - "langchain-core", "langchain", "langchain-openai", "langgraph", "pydantic", "python-dotenv", "ipykernel>=6.30.1", ]
14-26: Add conservative upper bounds to fast‑moving libs (langchain, langgraph, plotly)
pyproject.toml contains these deps (lines 16–22); add conservative upper bounds to prevent breaks from major upstream releases. I can propose concrete ranges after verifying compatibility.Makefile (2)
12-19: Prefercommand -voverwhichand avoid curl|sh where possible.Minor portability nit; and consider pointing devs to uv’s install docs instead of piping to sh.
install: @echo "Installing plot-agent for local development..." @echo "Installing uv if not present..." - @which uv > /dev/null || (echo "Installing uv..." && curl -LsSf https://astral.sh/uv/install.sh | sh) + @command -v uv >/dev/null 2>&1 || (echo "Installing uv..." && curl -LsSf https://astral.sh/uv/install.sh | sh)
33-36: Add -v to test output for parity with CI.Keeps local runs consistent with the workflow’s verbosity.
test: @echo "Running tests..." - uv run pytest + uv run pytest -vplot_agent/constants.py (2)
104-106: Mention uv in the optional dependency help text.Aligns with the project’s uv workflow.
POSTHOG_IMPORT_ERROR = ( - "PostHog integration requires the optional 'posthog' package. Install it with `pip install posthog`." + "PostHog integration requires the optional 'posthog' package. Install with `uv sync --extra posthog` or `pip install posthog`." )
22-29: Optional: document/install parity for allowed modules not in base deps.sklearn/scipy are allowed but not required in pyproject; consider a docs note or an extra to avoid surprises.
conftest.py (1)
6-13: Set a dummy OPENAI_API_KEY for hermetic tests.Prevents local test failures when the env isn’t pre‑configured.
@pytest.fixture(autouse=True) def clear_posthog_env(monkeypatch): """Clear PostHog environment variables for all tests to avoid conflicts.""" + monkeypatch.setenv("OPENAI_API_KEY", "test-key") monkeypatch.setenv("POSTHOG_LLM_ANALYTICS", "") monkeypatch.setenv("POSTHOG_PUBLIC_KEY", "") monkeypatch.setenv("POSTHOG_PROJECT_API_KEY", "") monkeypatch.setenv("POSTHOG_API_KEY", "") monkeypatch.setenv("POSTHOG_HOST", "")tests/unit/test_plot_execution.py (1)
67-67: Make the assertion resilient across Python versions by checking standardized prefixes.Avoids coupling to exact SyntaxError text/line numbers.
+from plot_agent.constants import CODE_EXECUTION_ERROR_PREFIX, CODE_REJECTED_ERROR_PREFIX @@ - assert "Error executing code: Code rejected on safety grounds: '(' was never closed (<unknown>, line 2)" in result + assert f"{CODE_EXECUTION_ERROR_PREFIX}{CODE_REJECTED_ERROR_PREFIX}" in resulttests/unit/test_initialization.py (3)
52-63: Minor: favor constants for env var names to avoid drift.Keeps tests aligned with constants if names change.
+from plot_agent.constants import POSTHOG_LLM_ANALYTICS_ENV_VAR @@ - monkeypatch.setenv("POSTHOG_PUBLIC_KEY", "") - monkeypatch.setenv("POSTHOG_PROJECT_API_KEY", "") - monkeypatch.setenv("POSTHOG_API_KEY", "") - monkeypatch.setenv("POSTHOG_HOST", "") + for k in ("POSTHOG_PUBLIC_KEY","POSTHOG_PROJECT_API_KEY","POSTHOG_API_KEY","POSTHOG_HOST"): + monkeypatch.setenv(k, "")
34-37: Nit: useis Falsefor boolean asserts.- assert agent.verbose == False + assert agent.verbose is False
28-31: Future‑proof model property assertion.Some langchain-openai versions expose
.modelinstead of.model_name.- assert agent.llm.model_name == "gpt-3.5-turbo" + model_name = getattr(agent.llm, "model_name", getattr(agent.llm, "model", None)) + assert model_name == "gpt-3.5-turbo"plot_agent/__init__.py (1)
16-24: Sort all to satisfy Ruff (RUF022).Minor style nit only.
Apply:
-__all__ = [ - "PlotAgent", - "DEFAULT_MODEL", - "DEFAULT_VERBOSE", - "DEFAULT_MAX_ITERATIONS", - "DEFAULT_LLM_TEMPERATURE", - "DEFAULT_LLM_TIMEOUT", - "DEFAULT_LLM_MAX_RETRIES", -] +__all__ = [ + "DEFAULT_LLM_MAX_RETRIES", + "DEFAULT_LLM_TEMPERATURE", + "DEFAULT_LLM_TIMEOUT", + "DEFAULT_MAX_ITERATIONS", + "DEFAULT_MODEL", + "DEFAULT_VERBOSE", + "PlotAgent", +]README.md (2)
57-61: Prefer new public import path in examples.Docs currently show from plot_agent.agent import PlotAgent. Since PlotAgent is re-exported, recommend from plot_agent import PlotAgent.
-from plot_agent.agent import PlotAgent +from plot_agent import PlotAgent
183-221: Docs imply constructor args alone enable PostHog, but code also requires POSTHOG_LLM_ANALYTICS.Either: (a) change code to treat explicit posthog_* args as opt-in (recommended; see callbacks.py comment), or (b) add a note that POSTHOG_LLM_ANALYTICS=true is still required when passing keys/clients.
plot_agent/execution.py (4)
9-10: Docstring still claims “60 second” timeout; constant now drives it.Make the comments reflect DEFAULT_TIMEOUT_SECONDS to avoid confusion.
- • Enforce a 60 second timeout via signal.alarm + • Enforce a configurable timeout via signal.alarm (see DEFAULT_TIMEOUT_SECONDS) @@ - • Enforce a 60 second timeout via signal.alarm + • Enforce a configurable timeout via signal.alarm (see DEFAULT_TIMEOUT_SECONDS)Also applies to: 64-71, 74-75
41-43: Name unused signal handler args to underscore to satisfy linters.No behavior change.
-def _timeout_handler(signum, frame): +def _timeout_handler(_signum, _frame): raise TimeoutError(TIMEOUT_ERROR_MESSAGE)
224-226: Avoid duplicating timeout message prefix.TIMEOUT_ERROR_MESSAGE is already the prefix; appending “: {te}” can repeat the same text. Keep prefix + traceback.
- "error": f"{TIMEOUT_ERROR_MESSAGE}: {te}\n{tb}", + "error": f"{TIMEOUT_ERROR_MESSAGE}\n{tb}",
289-297: Consider returning captured stdout on success.You currently replace user stdout with a static success message, which can make debugging harder. Consider adding stdout to output and moving the static text to a separate “message” field (non‑breaking if you keep output too).
plot_agent/callbacks.py (1)
75-83: Be robust to handler import path drift.PostHog’s LLM integrations are evolving. Prefer a fallback import to avoid breakage across minor SDK updates.
- try: - Posthog, PosthogCallbackHandler = _load_posthog_handler() - except ImportError as exc: - raise ImportError(POSTHOG_IMPORT_ERROR) from exc + try: + Posthog, PosthogCallbackHandler = _load_posthog_handler() + except ImportError as exc: + # Give ourselves one more chance under an alternate path before surfacing the friendly error + try: + from posthog import Posthog # type: ignore + from posthog.ai.langchain import CallbackHandler as PosthogCallbackHandler # type: ignore + except Exception as inner_exc: + raise ImportError(POSTHOG_IMPORT_ERROR) from excplot_agent/agent.py (2)
108-111: Treat PLOT_AGENT_DEBUG env as truthy flag, not just “1”.Tiny DX improvement; recognize common true values.
- self.debug = debug or os.getenv(PLOT_AGENT_DEBUG_ENV_VAR) == "1" + self.debug = debug or os.getenv(PLOT_AGENT_DEBUG_ENV_VAR, "").lower() in ("1", "true", "yes", "on")
178-184: Prefer explicit exceptions over asserts for user input validation.Asserts can be stripped with PYTHONOPTIMIZE. Consider raising ValueError/TypeError with the same messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.env.example(1 hunks).github/workflows/test.yml(1 hunks)Makefile(2 hunks)README.md(3 hunks)conftest.py(1 hunks)examples/run_log.txt(1 hunks)plot_agent/__init__.py(1 hunks)plot_agent/agent.py(17 hunks)plot_agent/callbacks.py(1 hunks)plot_agent/constants.py(1 hunks)plot_agent/execution.py(9 hunks)pyproject.toml(1 hunks)requirements-dev.txt(0 hunks)requirements.txt(0 hunks)tests/unit/test_initialization.py(2 hunks)tests/unit/test_plot_execution.py(1 hunks)
💤 Files with no reviewable changes (2)
- requirements-dev.txt
- requirements.txt
🧰 Additional context used
🧬 Code graph analysis (3)
plot_agent/__init__.py (1)
plot_agent/agent.py (1)
PlotAgent(66-520)
tests/unit/test_initialization.py (1)
plot_agent/agent.py (1)
PlotAgent(66-520)
plot_agent/agent.py (2)
plot_agent/callbacks.py (3)
create_callback_manager(119-147)has_callbacks(114-116)get_callbacks(110-112)plot_agent/models.py (3)
GeneratedCodeInput(16-21)DoesFigExistInput(24-27)ViewGeneratedCodeInput(30-33)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 6-6: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 6-6: [UnorderedKey] The LANGSMITH_ENDPOINT key should go before the LANGSMITH_TRACING key
(UnorderedKey)
[warning] 7-7: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 7-7: [UnorderedKey] The LANGSMITH_API_KEY key should go before the LANGSMITH_ENDPOINT key
(UnorderedKey)
[warning] 8-8: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 8-8: [UnorderedKey] The LANGSMITH_PROJECT key should go before the LANGSMITH_TRACING key
(UnorderedKey)
[warning] 12-12: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 13-13: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 13-13: [UnorderedKey] The POSTHOG_HOST key should go before the POSTHOG_LLM_ANALYTICS key
(UnorderedKey)
🪛 Ruff (0.12.2)
plot_agent/__init__.py
16-24: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
tests/unit/test_initialization.py
72-72: Avoid specifying long messages outside the exception class
(TRY003)
plot_agent/callbacks.py
88-90: Avoid specifying long messages outside the exception class
(TRY003)
plot_agent/execution.py
41-41: Unused function argument: signum
(ARG001)
41-41: Unused function argument: frame
(ARG001)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
🪛 YAMLlint (1.37.1)
.github/workflows/test.yml
[error] 34-34: no new line character at the end of file
(new-line-at-end-of-file)
🪛 checkmake (0.2.2)
Makefile
[warning] 3-3: Target body for "help" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 12-12: Target body for "install" exceeds allowed length of 5 (13).
(maxbodylength)
[warning] 46-46: Missing required phony target "all"
(minphony)
🔇 Additional comments (8)
examples/run_log.txt (1)
22-26: Confirm PostHog instrumentation is non‑blocking, batched, and timed out — logs inconclusive.
- Log analysis: 11 notebooks; sum durations 1604.80s; avg 145.89s; slowest 277.00s; wall‑clock 561.35s; summary lines match.
- Verify analytics client/config: ensure PostHog calls are async/queued or batched, use short timeouts (≤1s) and limited retries, and do not synchronously flush on every event; reproduce the run with analytics disabled and compare wall‑clock to quantify overhead.
.github/workflows/test.yml (1)
21-25: LGTM: uv setup in CI is straightforward and aligned with Makefile.tests/unit/test_initialization.py (1)
49-50: LGTM: early_stopping_method assertion matches the constructor arg.plot_agent/constants.py (1)
6-6: DEFAULT_MODEL = "gpt-5-mini" is valid — keep it. OpenAI documents gpt-5, gpt-5-mini, and gpt-5-nano as available API models and states these models support tool usage and core API features. (openai.com)plot_agent/__init__.py (2)
5-13: Public API re-exports look good.Clear surface with PlotAgent and DEFAULT_* constants; no behavior change.
15-15: Version synchronized — no action required.
pyproject.toml (line 7) and plot_agent/init.py (line 15) both specify version "0.4.0".plot_agent/agent.py (1)
218-239: Execution error messaging is consistent; nice.Prefix/suffix handling matches constants and aligns with updated tests.
plot_agent/callbacks.py (1)
15-20: Verify PostHog LangChain handler import
CI sandbox didn’t have PostHog installed—manually confirm that the installed PostHog SDK (per your CI and docs) exposesposthog.ai.langchain.CallbackHandler; if it lives under a different module, update the import accordingly.
| if self.callback_manager.has_callbacks(): | ||
| llm_kwargs["callbacks"] = self.callback_manager.get_callbacks() | ||
|
|
||
| self.llm = ChatOpenAI(**llm_kwargs) |
There was a problem hiding this comment.
Avoid double‑emitting callbacks; pass them only at request time.
You attach callbacks on the ChatOpenAI constructor and again via invoke config, which can cause duplicate events. LangChain recommends request‑time callbacks for propagation. Keep them on invoke_config only. (python.langchain.com)
- if self.callback_manager.has_callbacks():
- llm_kwargs["callbacks"] = self.callback_manager.get_callbacks()
+ # Callbacks are applied at request time for full propagation to child runs.
@@
- invoke_config = {"recursion_limit": self.max_iterations}
+ invoke_config = {"recursion_limit": self.max_iterations}
if self.callback_manager.has_callbacks():
invoke_config["callbacks"] = self.callback_manager.get_callbacks()
@@
- retry_config = {"recursion_limit": max(3, self.max_iterations // 2)}
+ retry_config = {"recursion_limit": max(3, self.max_iterations // 2)}
if self.callback_manager.has_callbacks():
retry_config["callbacks"] = self.callback_manager.get_callbacks()Also applies to: 399-406, 456-465
🤖 Prompt for AI Agents
In plot_agent/agent.py around lines 146-149 (and similarly at 399-406 and
456-465), you are attaching callbacks both on the ChatOpenAI constructor and
again at request/invoke time which causes duplicate events; remove the code that
injects "callbacks" into the llm_kwargs passed to ChatOpenAI so the LLM is
constructed without callbacks, and ensure any callback propagation is done only
by supplying callbacks in the invoke_config (or request-time call) paths; update
the three locations so callbacks are only passed at request time and not in the
ChatOpenAI constructor.
| # Check if PostHog analytics is enabled | ||
| if not self._is_posthog_enabled(): | ||
| return False | ||
|
|
There was a problem hiding this comment.
Don’t require POSTHOG_LLM_ANALYTICS when explicit key/client is provided.
Constructor args should be an explicit opt‑in; current gate returns False even if a key/client is passed, contradicting README.
- # Check if PostHog analytics is enabled
- if not self._is_posthog_enabled():
- return False
+ # If no explicit credentials/client, require env opt-in
+ if not (posthog_public_key or posthog_client) and not self._is_posthog_enabled():
+ return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check if PostHog analytics is enabled | |
| if not self._is_posthog_enabled(): | |
| return False | |
| # If no explicit credentials/client, require env opt-in | |
| if not (posthog_public_key or posthog_client) and not self._is_posthog_enabled(): | |
| return False |
| CODE_EXECUTION_SUCCESS_PREFIX = "Success: " | ||
| CODE_EXECUTION_ERROR_PREFIX = "Error: " | ||
| CODE_EXECUTION_SUCCESS_MESSAGE = "Code executed successfully. 'fig', 'plot_title', and 'plot_summary' objects were created." | ||
|
|
There was a problem hiding this comment.
Duplicate constant: CODE_EXECUTION_ERROR_PREFIX is defined twice with different values.
This is confusing and error‑prone. Keep the “Error executing code: ” variant once.
# Code Execution Messages
CODE_EXECUTION_SUCCESS_PREFIX = "Success: "
-CODE_EXECUTION_ERROR_PREFIX = "Error: "
-CODE_EXECUTION_SUCCESS_MESSAGE = "Code executed successfully. 'fig', 'plot_title', and 'plot_summary' objects were created."
+CODE_EXECUTION_ERROR_PREFIX = "Error executing code: "
+CODE_EXECUTION_SUCCESS_MESSAGE = "Code executed successfully. 'fig', 'plot_title', and 'plot_summary' objects were created."
@@
TIMEOUT_ERROR_MESSAGE = "Code execution timed out"
CODE_REJECTED_ERROR_PREFIX = "Code rejected on safety grounds: "
-CODE_EXECUTION_ERROR_PREFIX = "Error executing code: "Also applies to: 108-112
🤖 Prompt for AI Agents
In plot_agent/constants.py around lines 93-96 and again at 108-112, the constant
CODE_EXECUTION_ERROR_PREFIX is defined twice with conflicting values; remove the
duplicate and ensure there is a single definition using the desired value "Error
executing code: " (remove the other variant, update any import/usage if
necessary to reference the single constant) so the module only exports one
consistent error prefix.
- Remove callbacks from ChatOpenAI constructor to prevent duplicate events - Keep callbacks only at request time via invoke_config for proper propagation - Add comment explaining request-time callback usage - Follows LangChain best practices for callback handling Fixes callback duplication issue identified in code review.
- Refactor the validation of required execution variables to use a dedicated dictionary for extracted variables. - This change improves clarity and ensures that the validation logic correctly checks for missing variables. This update enhances the robustness of the execution environment by ensuring all required variables are properly validated.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
plot_agent/execution.py (1)
258-262: Good fix: validate required variables from the exec namespace.
This addresses the prior “locals()” scope issue and checks fig/title/summary correctly.plot_agent/agent.py (1)
140-147: Callbacks only at request time — good correction.
No callbacks are bound on the ChatOpenAI constructor; avoids double-emission.
🧹 Nitpick comments (8)
plot_agent/execution.py (4)
41-42: Silence Ruff ARG001 by naming unused signal handler args with underscores.Apply:
-def _timeout_handler(signum, frame): +def _timeout_handler(_signum, _frame): raise TimeoutError(TIMEOUT_ERROR_MESSAGE)
224-225: Include stderr and avoid duplicating the timeout message.
Currently the error repeats the message and drops captured stderr.Apply:
- "error": f"{TIMEOUT_ERROR_MESSAGE}: {te}\n{tb}", + "error": f"{TIMEOUT_ERROR_MESSAGE}\n{err_buf.getvalue()}{tb}",
235-236: Surface stderr in execution errors.
err_buf is captured but never returned; include it for better diagnostics.Apply:
- "error": f"{CODE_EXECUTION_ERROR_PREFIX}{e}\n{tb}", + "error": f"{CODE_EXECUTION_ERROR_PREFIX}{e}\n{err_buf.getvalue()}{tb}",
151-157: Minor: prefer concise ValueError text or a custom exception type.
Ruff TRY003 suggests avoiding long messages in raises; consider a custom ValidationError or shorter text.plot_agent/agent.py (4)
216-216: Same here: replace assert with explicit TypeError.Apply:
- assert isinstance(generated_code, str), INVALID_CODE_TYPE_ERROR + if not isinstance(generated_code, str): + raise TypeError(INVALID_CODE_TYPE_ERROR)
306-311: Use StructuredTool when supplying args_schema.
Ensures LangChain validates inputs and schema consistently.Apply:
- Tool.from_function( + StructuredTool.from_function( func=self.execute_plotly_code, name=TOOL_EXECUTE_PLOTLY_CODE, description=TOOL_DESCRIPTIONS[TOOL_EXECUTE_PLOTLY_CODE], args_schema=GeneratedCodeInput, ),
110-111: Support common truthy values for debug env.
Treat “true/yes/on/1” as enabled.Apply:
- self.debug = debug or os.getenv(PLOT_AGENT_DEBUG_ENV_VAR) == "1" + self.debug = debug or os.getenv(PLOT_AGENT_DEBUG_ENV_VAR, "").strip().lower() in {"1","true","yes","on"}
495-509: Truthiness checks can misfire; compare to None explicitly.
Prevents surprises with objects that define bool or empty strings.Apply:
- if self.execution_env and self.execution_env.fig: + if self.execution_env and self.execution_env.fig is not None: @@ - if self.execution_env and self.execution_env.plot_title: + if self.execution_env and self.execution_env.plot_title is not None: @@ - if self.execution_env and self.execution_env.plot_summary: + if self.execution_env and self.execution_env.plot_summary is not None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plot_agent/agent.py(17 hunks)plot_agent/execution.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plot_agent/agent.py (2)
plot_agent/callbacks.py (3)
create_callback_manager(119-147)has_callbacks(114-116)get_callbacks(110-112)plot_agent/models.py (3)
GeneratedCodeInput(16-21)DoesFigExistInput(24-27)ViewGeneratedCodeInput(30-33)
🪛 Ruff (0.12.2)
plot_agent/execution.py
41-41: Unused function argument: signum
(ARG001)
41-41: Unused function argument: frame
(ARG001)
152-152: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
plot_agent/agent.py (1)
397-400: Request-time callback propagation looks correct.
Callbacks are added to invoke/retry configs only when present.Also applies to: 456-459
| # Check df | ||
| assert isinstance(df, pd.DataFrame), "The dataframe must be a pandas dataframe." | ||
| assert not df.empty, "The dataframe must not be empty." | ||
| assert isinstance(df, pd.DataFrame), INVALID_DF_TYPE_ERROR | ||
| assert not df.empty, EMPTY_DF_ERROR | ||
|
|
||
| if sql_query: | ||
| assert isinstance(sql_query, str), "The SQL query must be a string." | ||
| assert isinstance(sql_query, str), INVALID_SQL_TYPE_ERROR | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Repo-wide check for remaining asserts.
Ensure no other runtime validations rely on assert.
Also applies to: 216-216
🏁 Script executed:
#!/bin/bash
# List Python asserts across the repo with context.
rg -nP --type=py -C2 '^\s*assert\b'Length of output: 21445
Replace runtime asserts in production code with explicit exceptions.
Assertions can be disabled with -O — do not rely on them for runtime validation.
- plot_agent/agent.py:176–182 — replace asserts with explicit checks: raise TypeError(INVALID_DF_TYPE_ERROR) / raise ValueError(EMPTY_DF_ERROR) / if sql_query is not None: raise TypeError(INVALID_SQL_TYPE_ERROR).
- plot_agent/agent.py:214–216 — replace assert with: raise TypeError(INVALID_CODE_TYPE_ERROR).
- plot_agent/agent.py:358–360 — replace assert with: raise TypeError(INVALID_MESSAGE_TYPE_ERROR).
Asserts in tests are fine to keep.
🤖 Prompt for AI Agents
In plot_agent/agent.py around lines 176–182, 214–216 and 358–360, replace
runtime assert statements with explicit exceptions: at 176–182 change the df
checks to explicit validations that raise TypeError(INVALID_DF_TYPE_ERROR) if df
is not a pandas DataFrame and ValueError(EMPTY_DF_ERROR) if df.empty; for the
sql_query check only validate when sql_query is not None and raise
TypeError(INVALID_SQL_TYPE_ERROR) if it is not a str; at 214–216 replace the
assert with raise TypeError(INVALID_CODE_TYPE_ERROR); and at 358–360 replace the
assert with raise TypeError(INVALID_MESSAGE_TYPE_ERROR), ensuring these are
plain runtime checks used instead of assert so they cannot be disabled.
Don’t use assert for input validation; raise explicit exceptions.
Asserts can be stripped with -O, disabling checks in production.
Apply:
- assert isinstance(df, pd.DataFrame), INVALID_DF_TYPE_ERROR
- assert not df.empty, EMPTY_DF_ERROR
+ if not isinstance(df, pd.DataFrame):
+ raise TypeError(INVALID_DF_TYPE_ERROR)
+ if df.empty:
+ raise ValueError(EMPTY_DF_ERROR)
@@
- if sql_query:
- assert isinstance(sql_query, str), INVALID_SQL_TYPE_ERROR
+ if sql_query is not None and not isinstance(sql_query, str):
+ raise TypeError(INVALID_SQL_TYPE_ERROR)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check df | |
| assert isinstance(df, pd.DataFrame), "The dataframe must be a pandas dataframe." | |
| assert not df.empty, "The dataframe must not be empty." | |
| assert isinstance(df, pd.DataFrame), INVALID_DF_TYPE_ERROR | |
| assert not df.empty, EMPTY_DF_ERROR | |
| if sql_query: | |
| assert isinstance(sql_query, str), "The SQL query must be a string." | |
| assert isinstance(sql_query, str), INVALID_SQL_TYPE_ERROR | |
| # Check df | |
| if not isinstance(df, pd.DataFrame): | |
| raise TypeError(INVALID_DF_TYPE_ERROR) | |
| if df.empty: | |
| raise ValueError(EMPTY_DF_ERROR) | |
| if sql_query is not None and not isinstance(sql_query, str): | |
| raise TypeError(INVALID_SQL_TYPE_ERROR) |
🤖 Prompt for AI Agents
In plot_agent/agent.py around lines 176 to 182, the code currently uses Python
assert statements for input validation (checking df type, emptiness, and
sql_query type); replace these asserts with explicit exceptions so checks cannot
be stripped in optimized mode. Specifically, check: if not isinstance(df,
pd.DataFrame): raise TypeError(INVALID_DF_TYPE_ERROR); if df.empty: raise
ValueError(EMPTY_DF_ERROR); and if sql_query is not None and not
isinstance(sql_query, str): raise TypeError(INVALID_SQL_TYPE_ERROR). Keep the
existing error message constants and preserve the same order of checks.
| if code_execution_success: | ||
| return f"Success: {code_execution_output}" | ||
| return f"{CODE_EXECUTION_SUCCESS_PREFIX}{code_execution_output}" | ||
| else: | ||
| return f"Error: {code_execution_error}\n{code_execution_output}" | ||
| return f"{CODE_EXECUTION_ERROR_PREFIX}{code_execution_error}\n{code_execution_output}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Downstream UX depends on execution.py returning stdout in “output”.
If you adopt the suggested change in execution.py, this will return meaningful logs; otherwise it returns a fixed success string. Confirm intended contract.
Run:
🏁 Script executed:
#!/bin/bash
# Inspect all uses of `execute_code(...)[ "output" ]` to verify assumptions.
rg -nP --type=py -C2 '\.execute_code\([^)]+\)\.get\(["\']output["\']'Length of output: 68
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== plot_agent/agent.py (lines 220-260) ====="
if [[ -f plot_agent/agent.py ]]; then
sed -n '1,260p' plot_agent/agent.py | sed -n '220,260p'
else
echo "plot_agent/agent.py not found"
fi
echo
echo "===== Searching for execute_code definition ====="
rg -n --type=py 'def\s+execute_code' -S || true
echo
echo "===== Searching for execute_code usages ====="
rg -n --type=py 'execute_code\(' -S || true
echo
echo "===== Searching for .get(\"output\") usages ====="
rg -n --type=py '\.get\("output"\)' -S || true
echo
echo "===== Searching for .get('output') usages ====="
rg -n --type=py "\.get\('output'\)" -S || true
echo
echo "===== Searching for indexing [\"output\"] or ['output'] usages ====="
rg -n --type=py '\["output"\]' -S || true
rg -n --type=py "\['output'\]" -S || trueLength of output: 2412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== plot_agent/execution.py (lines 1-400) ====="
if [[ -f plot_agent/execution.py ]]; then
sed -n '1,400p' plot_agent/execution.py
else
echo "plot_agent/execution.py not found"
fi
echo
echo "===== plot_agent/execution.py (lines 140-220) - around execute_code ====="
if [[ -f plot_agent/execution.py ]]; then
sed -n '140,220p' plot_agent/execution.py
fi
echo
echo "===== return {...} occurrences in execution.py ====="
rg -n "return\s+{" plot_agent/execution.py || true
echo
echo "===== 'output' occurrences in execution.py ====="
rg -n --hidden --no-ignore 'output' plot_agent/execution.py || true
echo
echo "===== 'stdout' occurrences in execution.py ====="
rg -n --hidden --no-ignore 'stdout' plot_agent/execution.py || trueLength of output: 14141
Confirm contract: should execute_code return captured stdout in "output" on success?
execute_code currently sets "output" to CODE_EXECUTION_SUCCESS_MESSAGE on success (plot_agent/execution.py — final return, ~line 295) while agent.py reads code_execution_result.get("output") for downstream UX (plot_agent/agent.py lines 233–237). Either change execution.py to return "output": out_buf.getvalue() on success so callers receive real logs, or update the agent to stop relying on stdout.
🤖 Prompt for AI Agents
In plot_agent/agent.py around lines 233–237 the agent expects execute_code to
return the real captured stdout in code_execution_result["output"], but
execution.py currently sets "output" to a generic success message on successful
runs; modify plot_agent/execution.py so that on success it returns "output":
out_buf.getvalue() (the captured stdout) instead of the generic
CODE_EXECUTION_SUCCESS_MESSAGE, and keep CODE_EXECUTION_SUCCESS_MESSAGE only as
a separate status field if needed; alternatively, if you prefer changing the
agent, update plot_agent/agent.py to not rely on "output" for success paths and
instead use a separate "status" or "message" field—pick one approach and make
both caller and callee consistent.
| "output": CODE_EXECUTION_SUCCESS_MESSAGE, | ||
| "error": "", |
There was a problem hiding this comment.
Return actual stdout on success; keep status in a separate field.
Docstring says output is captured stdout; today it returns a status string.
Apply:
- "output": CODE_EXECUTION_SUCCESS_MESSAGE,
+ "output": out_buf.getvalue(),
+ "message": CODE_EXECUTION_SUCCESS_MESSAGE,If adding a new key is risky for callers, at minimum set output to out_buf.getvalue().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "output": CODE_EXECUTION_SUCCESS_MESSAGE, | |
| "error": "", | |
| "output": out_buf.getvalue(), | |
| "message": CODE_EXECUTION_SUCCESS_MESSAGE, | |
| "error": "", |
🤖 Prompt for AI Agents
In plot_agent/execution.py around lines 295-296, the handler currently sets
"output" to a status string (CODE_EXECUTION_SUCCESS_MESSAGE) instead of the
captured stdout; change the dict so "output" is set to the captured stdout
(e.g., out_buf.getvalue()) and move the status into a separate key (e.g.,
"status" or "success": True). If adding a new key is judged risky for callers,
at minimum set "output" to out_buf.getvalue() and leave the existing status key
unchanged.
This pull request introduces optional PostHog LLM analytics integration, improves developer experience with new Makefile targets and documentation, and refactors the
PlotAgentclass for better configuration and error handling. It also updates the workflow to useuvfor dependency management and testing, and adds test fixtures for cleaner test environments.PostHog LLM analytics integration:
.env.example,README.md, andPlotAgentclass, allowing agent traces to be streamed to PostHog via LangChain callbacks. New arguments and documentation are provided for easy setup and customization. [1] [2] [3] [4] [5]plot_agent/callbacks.py(not shown) and a pytest fixture inconftest.pyto clear PostHog-related environment variables for reliable testing.Developer experience improvements:
installtarget to theMakefilefor setting up local development, including installinguvand dependencies. Added ahelptarget and improved documentation inREADME.mdto guide developers through setup and available make targets. [1] [2]uvfor dependency management and running tests/scripts, replacing pip for faster and more reproducible installs. [1] [2]PlotAgent class refactoring and error handling:
PlotAgentto use constants for default values and error messages, added new configuration options for analytics, improved error handling and logging, and exposed analytics properties for backward compatibility. [1] [2] [3] [4] [5] [6] [7]plot_agent/__init__.pyto expose public API and constants, and set version information.Documentation and usage updates:
README.mdwith developer setup instructions, usage of new make targets, and detailed instructions for enabling PostHog analytics. [1] [2] [3]Testing and logs:
conftest.py).Let me know if you'd like a walkthrough of the new analytics integration or help with the updated developer workflow!
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Tests