Skip to content

Update dependencies and sync branches for environment files [possibly breaking!]#168

Closed
lfnothias wants to merge 11 commits into
mainfrom
dev
Closed

Update dependencies and sync branches for environment files [possibly breaking!]#168
lfnothias wants to merge 11 commits into
mainfrom
dev

Conversation

@lfnothias

@lfnothias lfnothias commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

User description

Important

This is done to resolve several critical security issue identified by DependatBot but this might be breaking the library. @madina1203 can you please run some tests to evaluate that.

Summary

This PR promotes the dependency updates currently on dev into main.

The changes are limited to dependency manifests:

  • requirements.txt
  • environment.yml

Main updates

This refresh upgrades several core libraries used by MetaboT, including:

  • langchain 0.1.11 -> 0.3.27
  • langchain_community 0.0.27 -> 0.3.31
  • langchain_core 0.1.30 -> 0.3.84
  • langchain_openai 0.0.8 -> 0.3.35
  • langgraph 0.0.26 -> 0.3.34
  • litellm 1.61.6 -> 1.83.0
  • langsmith 0.1.22 -> 0.3.45
  • openai 1.61.0 -> 2.32.0
  • pydantic 2.7.0 -> 2.13.3
  • numpy 1.26.0 -> 1.26.4

Additional package updates include:

  • python-dotenv 1.0.1 -> 1.2.2
  • requests 2.31.0 -> 2.33.1
  • streamlit 1.44.0 -> 1.54.0
  • tqdm 4.66.1 -> 4.66.6

Notes

  • No application code changes are included in this PR.
  • The goal is to keep the project aligned with newer LLM ecosystem dependencies and maintain consistency between requirements.txt and environment.yml.

Testing

  • Dependency manifest update only
  • No code changes included in this branch

PR Type

enhancement, dependencies


Description

  • Update multiple dependencies in environment.yml

  • Align requirements.txt with environment.yml

  • Resolve critical security issues via dependency updates

  • Promote dependency updates from dev to main


Diagram Walkthrough

flowchart LR
  A["Update dependencies"] -- "langchain, openai, etc." --> B["environment.yml"]
  A -- "align with environment.yml" --> C["requirements.txt"]
  B -- "promote to main" --> D["Resolve security issues"]
Loading

File Walkthrough

Relevant files
Dependencies
environment.yml
Update dependencies in `environment.yml` for security       

environment.yml

  • Updated langchain to version 0.3.27
  • Updated openai to version 2.32.0
  • Updated pydantic to version 2.13.3
  • Updated several other dependencies
+13/-13 


Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @lfnothias
    lfnothias requested a review from madina1203 April 23, 2026 15:30
    @dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Apr 23, 2026
    @coderabbitai

    coderabbitai Bot commented Apr 23, 2026

    Copy link
    Copy Markdown

    Warning

    Rate limit exceeded

    @lfnothias has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 55 seconds before requesting another review.

    Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 55 seconds.

    ⌛ How to resolve this issue?

    After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

    We recommend that you space out your commits to avoid hitting the rate limit.

    🚦 How do rate limits work?

    CodeRabbit enforces hourly rate limits for each developer per organization.

    Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

    Please see our FAQ for further information.

    ℹ️ Review info
    ⚙️ Run configuration

    Configuration used: Repository UI

    Review profile: CHILL

    Plan: Pro

    Run ID: f7ce2d2e-1281-4f54-b00e-4f257ce91bd0

    📥 Commits

    Reviewing files that changed from the base of the PR and between 085c973 and 8adb7a3.

    ⛔ Files ignored due to path filters (2)
    • app/config/params.ini is excluded by !**/*.ini
    • environment.yml is excluded by !**/*.yml
    📒 Files selected for processing (5)
    • app/core/agents/enpkg/tool_taxon.py
    • app/core/agents/interpreter/agent.py
    • app/core/agents/interpreter/tool_interpreter.py
    • app/core/memory/custom_sqlite_file.py
    • requirements.txt

    Walkthrough

    This pull request migrates Pydantic imports across multiple tool modules from LangChain's bundled versions to direct pydantic, adds explicit type annotations to args_schema class attributes, and updates dependencies to newer versions. It also introduces fallback behavior in the memory manager and enhances evaluation dataset validation with auto-loading from local CSV.

    Changes

    Cohort / File(s) Summary
    Pydantic Import Migration: ENPKG Tools
    app/core/agents/enpkg/tool_chemicals.py, tool_smiles.py, tool_target.py, tool_taxon.py
    Switched BaseModel/Field imports from langchain.pydantic_v1 to pydantic. Added explicit type[BaseModel] annotations to args_schema attributes. tool_taxon.py also annotates ENDPOINT_URL and PREFIXES as ClassVar[str].
    Pydantic Import Migration: Entry & Interpreter Tools
    app/core/agents/entry/tool_filesparser.py, app/core/agents/interpreter/tool_interpreter.py, tool_spectrum.py
    Updated imports to use pydantic directly. Added type[BaseModel] type annotation to args_schema. tool_filesparser.py introduces new empty FileAnalyzerInput schema class.
    Pydantic Import Migration: SPARQL & Validator Tools
    app/core/agents/sparql/tool_merge_result.py, tool_sparql.py, tool_wikidata_query.py, app/core/agents/validator/tool_validator.py
    Migrated from langchain.pydantic_v1 to pydantic. Added type[BaseModel] annotation to args_schema. tool_sparql.py adds explicit type annotations to name, description, requires_params attributes. tool_wikidata_query.py marks ENDPOINT_URL and PREFIXES as ClassVar[str].
    Memory & Database Configuration
    app/core/memory/custom_sqlite_file.py, app/core/memory/database_manager.py
    Updated database_path to ClassVar[str] in SqliteCheckpointerSaver. Changed fallback memory saver from custom SqliteCheckpointerSaver to langgraph.checkpoint.memory.MemorySaver() when neither database URL nor custom class is configured.
    Evaluation & Testing Scripts
    app/core/tests/evaluation.py, app/tests/installation_test.py
    evaluation.py adds environment variable validation, LangSmith dataset existence checks, and auto-loads evaluation data from local CSV if dataset is missing. installation_test.py fixes repository root path resolution in sys.path and normalizes module execution guard syntax.
    Logging Improvement
    app/core/workflow/langraph_workflow.py
    Enhanced error logging with traceback information via exc_info=True parameter.
    Dependency Updates
    requirements.txt
    Upgraded LangChain ecosystem (langchain, langchain_community, langchain_core, langchain_openai, langgraph), OpenAI SDK (1.x2.x), pydantic, python-dotenv, requests, streamlit, numpy, and tqdm to newer versions.

    Estimated code review effort

    🎯 3 (Moderate) | ⏱️ ~20 minutes

    Suggested labels

    size:XXL

    Suggested reviewers

    • madina1203

    Poem

    🐰 From LangChain's bundles we hop away,
    To Pydantic pure, hip-hip-hooray!
    Type hints align, ClassVar so bright,
    Dependencies dance, the code feels so light! ✨

    🚥 Pre-merge checks | ✅ 4 | ❌ 1

    ❌ Failed checks (1 warning)

    Check name Status Explanation Resolution
    Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
    ✅ Passed checks (4 passed)
    Check name Status Explanation
    Title check ✅ Passed The title accurately reflects the main change: updating dependencies and syncing environment files between branches, with an explicit note about potential breaking changes.
    Description check ✅ Passed The description is directly related to the changeset, clearly explaining the dependency updates, security rationale, and specific version bumps across multiple packages.
    Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
    Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

    ✏️ Tip: You can configure your own custom pre-merge checks in the settings.

    ✨ Finishing Touches
    🧪 Generate unit tests (beta)
    • Create PR with unit tests
    • Commit unit tests in branch dev

    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.

    ❤️ Share

    Comment @coderabbitai help to get the list of available commands and usage tips.

    @qodo-code-review

    Copy link
    Copy Markdown

    Review Summary by Qodo

    Update dependencies to latest LLM ecosystem versions

    ✨ Enhancement

    Grey Divider

    Walkthroughs

    Description
    • Upgrade langchain ecosystem to latest versions (0.3.x)
    • Update OpenAI SDK from 1.61.0 to 2.32.0
    • Bump pydantic, litellm, and langsmith to latest releases
    • Update supporting libraries (python-dotenv, requests, streamlit, tqdm, numpy)
    
    Diagram
    flowchart LR
      A["Dependency Manifests"] -->|"langchain 0.1.11 → 0.3.27"| B["LangChain Ecosystem"]
      A -->|"openai 1.61.0 → 2.32.0"| C["OpenAI SDK"]
      A -->|"pydantic 2.7.0 → 2.13.3"| D["Core Libraries"]
      A -->|"litellm 1.61.6 → 1.83.0"| E["LLM Utilities"]
      B --> F["Updated environment.yml"]
      C --> F
      D --> F
      E --> F
      F -->|"Also updates"| G["Supporting packages"]
    
    Loading

    Grey Divider

    File Changes

    1. environment.yml Dependencies +13/-13

    Comprehensive dependency version upgrades

    • Upgraded langchain from 0.1.11 to 0.3.27
    • Updated langchain_community, langchain_core, langchain_openai, and langgraph to 0.3.x versions
    • Bumped OpenAI SDK from 1.61.0 to 2.32.0
    • Updated pydantic from 2.7.0 to 2.13.3
    • Upgraded litellm from 1.61.6 to 1.83.0 and langsmith from 0.1.22 to 0.3.45
    • Updated supporting libraries: python-dotenv, requests, streamlit, tqdm, and numpy
    

    environment.yml


    2. requirements.txt Dependencies +0/-0

    Comprehensive dependency version upgrades

    • Upgraded langchain from 0.1.11 to 0.3.27
    • Updated langchain_community, langchain_core, langchain_openai, and langgraph to 0.3.x versions
    • Bumped OpenAI SDK from 1.61.0 to 2.32.0
    • Updated pydantic from 2.7.0 to 2.13.3
    • Upgraded litellm from 1.61.6 to 1.83.0 and langsmith from 0.1.22 to 0.3.45
    • Updated supporting libraries: python-dotenv, requests, streamlit, tqdm, and numpy
    

    requirements.txt


    Grey Divider

    Qodo Logo


    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review

    qodo-code-review Bot commented Apr 23, 2026

    Copy link
    Copy Markdown

    Code Review by Qodo

    🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

    Grey Divider


    Action required

    1. Streamlit missing from conda env 🐞 Bug ☼ Reliability
    Description
    environment.yml does not install streamlit, but the streamlit_webapp modules import it at
    import-time; creating the conda env and running the webapp will raise `ImportError: No module named
    'streamlit'. This is especially risky here because requirements.txt pins streamlit==1.54.0`, so
    the two install paths currently disagree on a required runtime dependency.
    
    Code

    environment.yml[R39-45]

    +      - requests==2.33.1
         - seaborn==0.13.2
         - SPARQLWrapper==2.0.0
         - SQLAlchemy==2.0.22
         - tiktoken>=0.7.0
    -      - tqdm==4.66.1
    -      - numpy==1.26.0
    +      - tqdm==4.66.6
    +      - numpy==1.26.4
    Evidence
    The conda environment definition (environment.yml) contains the full pip dependency list and has
    no streamlit entry anywhere, while the Streamlit webapp imports streamlit unconditionally and
    requirements.txt includes it.
    

    environment.yml[1-48]
    requirements.txt[1-32]
    streamlit_webapp/streamlit_app.py[2-4]
    streamlit_webapp/streamlit_utils.py[1-7]

    Agent prompt
    The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
    
    ## Issue description
    `environment.yml` is missing `streamlit`, but `streamlit_webapp` imports it at module import time. This will break anyone using the conda environment to run the Streamlit app.
    ### Issue Context
    `requirements.txt` already pins `streamlit==1.54.0`, so adding the same pin to `environment.yml` also improves consistency between the two manifests.
    ### Fix Focus Areas
    - environment.yml[13-48]
    

    ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


    Grey Divider

    Qodo Logo

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Dependency Breakage

    The updated dependency versions, including major version bumps for packages like langchain, openai, and pydantic, could introduce incompatibilities with the existing codebase. Although this change is intended to resolve security issues, it is important to run comprehensive integration tests to verify that these updates do not break functionality. Also, note that while the PR extra instructions request Google DocString formatting for key functions and classes, no such code elements are present in this dependency manifest.

    - langchain==0.3.27
    - langchainhub==0.1.15
    - langchain_community==0.3.31
    - langchain_core==0.3.84
    - langchain_openai==0.3.35
    - langgraph==0.3.34
    - litellm==1.83.0
    - langsmith==0.3.45
    - matplotlib==3.8.3
    - networkx==3.2.1
    - openai==2.32.0
    - pandas==2.2.1
    - plotly==5.20.0
    - psycopg2-binary
    - pydantic==2.13.3
    - python-dotenv==1.2.2
    - rdflib==7.0.0
    - requests==2.33.1
    - seaborn==0.13.2
    - SPARQLWrapper==2.0.0
    - SQLAlchemy==2.0.22
    - tiktoken>=0.7.0
    - tqdm==4.66.6
    - numpy==1.26.4

    @github-actions

    github-actions Bot commented Apr 23, 2026

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    Comment thread environment.yml
    Comment on lines +39 to +45
    - requests==2.33.1
    - seaborn==0.13.2
    - SPARQLWrapper==2.0.0
    - SQLAlchemy==2.0.22
    - tiktoken>=0.7.0
    - tqdm==4.66.1
    - numpy==1.26.0
    - tqdm==4.66.6
    - numpy==1.26.4

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    Action required

    1. Streamlit missing from conda env 🐞 Bug ☼ Reliability

    environment.yml does not install streamlit, but the streamlit_webapp modules import it at
    import-time; creating the conda env and running the webapp will raise `ImportError: No module named
    'streamlit'. This is especially risky here because requirements.txt pins streamlit==1.54.0`, so
    the two install paths currently disagree on a required runtime dependency.
    
    Agent Prompt
    ### Issue description
    `environment.yml` is missing `streamlit`, but `streamlit_webapp` imports it at module import time. This will break anyone using the conda environment to run the Streamlit app.
    
    ### Issue Context
    `requirements.txt` already pins `streamlit==1.54.0`, so adding the same pin to `environment.yml` also improves consistency between the two manifests.
    
    ### Fix Focus Areas
    - environment.yml[13-48]
    

    ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

    @dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Apr 24, 2026

    @coderabbitai coderabbitai Bot left a comment

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    Actionable comments posted: 6

    Caution

    Some comments are outside the diff and can’t be posted inline due to platform limitations.

    ⚠️ Outside diff range comments (4)
    app/core/workflow/langraph_workflow.py (3)

    236-236: ⚠️ Potential issue | 🟠 Major

    Incorrect return type annotation: NoReturn should be None.

    The NoReturn type hint indicates a function never returns normally (e.g., always raises or has an infinite loop). However, process_workflow returns None implicitly after completing successfully or catching an exception (line 252-253). This misleads type checkers and developers about the function's behavior.

    🔧 Proposed fix
    -def process_workflow(app: StateGraph, question: str, thread_id: int = 1) -> NoReturn:
    +def process_workflow(app: StateGraph, question: str, thread_id: int = 1) -> None:
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/workflow/langraph_workflow.py` at line 236, The function
    process_workflow is annotated with NoReturn but it returns implicitly (or after
    handling exceptions); change its return type annotation from NoReturn to None in
    the process_workflow signature so type checkers and readers understand it can
    complete normally; update the def process_workflow(app: StateGraph, question:
    str, thread_id: int = 1) -> None signature accordingly.
    

    90-95: ⚠️ Potential issue | 🟠 Major

    Security: Unpickling without verification poses code execution risk.

    Loading pickled data with pickle.load can execute arbitrary code if the file is tampered with. While this may be acceptable in controlled environments, consider adding integrity verification (e.g., HMAC signature) or using a safer serialization format like JSON for the graph cache.

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/workflow/langraph_workflow.py` around lines 90 - 95, The code
    currently uses pickle.load on graph_path which can execute arbitrary code;
    modify the loader in langraph_workflow to verify integrity before unpickling
    (e.g., store and check an HMAC signature alongside the cached file using a
    secret key) or replace the cache format with a safe serializer (e.g., JSON or
    msgpack) and deserialize without pickle; specifically update the block that
    reads graph_path and creates graph (the pickle.load call that returns graph) to
    first validate the file signature or switch to a non-pickle read/parse routine
    so untrusted pickle data is never directly passed to pickle.load.
    

    111-111: ⚠️ Potential issue | 🔴 Critical

    Critical: Invalid parameter type annotation syntax.

    The parameter declaration evaluation=bool is invalid Python syntax. Type annotations require a colon, not an equals sign.

    🐛 Proposed fix
    -    evaluation=bool,
    +    evaluation: bool,
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/workflow/langraph_workflow.py` at line 111, The parameter
    declaration uses invalid syntax `evaluation=bool`; change it to a proper type
    annotation like `evaluation: bool` and, if a default is intended, provide one
    (e.g. `evaluation: bool = False`) in the function/method signature where
    `evaluation` appears (look for the function in langraph_workflow.py that
    contains the `evaluation` parameter).
    
    app/core/tests/evaluation.py (1)

    1-146: ⚠️ Potential issue | 🟡 Minor

    PR scope drift: this file contains substantive logic changes, not just dependency updates.

    The PR description states "no application code changes are included; testing notes indicate only dependency manifest updates," but this file adds new LangSmith dataset auto-provisioning, new env-var setup, new input-extraction logic, and changes the dataset name (test_metabot). Please either update the PR description/title to reflect the code changes, or split these changes out into a separate PR so the security-critical dependency bumps can be reviewed and merged independently.

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/tests/evaluation.py` around lines 1 - 146, The PR introduces
    functional changes in this test file (new env var setup, LangSmith dataset
    auto-provisioning, input-extraction logic, changed dataset_name) but the PR
    description claims only dependency updates; either update the PR
    title/description to clearly list these code changes (mentioning dataset_name,
    the env var assignments, dataset creation flow, and
    evaluate_result/create_workflow/run_on_dataset usage) or revert/split those
    changes into a separate PR focused on application logic so the dependency-only
    PR remains limited to manifest updates.
    
    🧹 Nitpick comments (7)
    app/core/workflow/langraph_workflow.py (1)

    204-204: Prefer idiomatic boolean check.

    Explicitly comparing to True is not idiomatic in Python. Consider using the boolean value directly.

    ♻️ Proposed simplification
    -    if evaluation == True:
    +    if evaluation:
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/workflow/langraph_workflow.py` at line 204, The condition "if
    evaluation == True:" is non-idiomatic; update the check in langraph_workflow.py
    to use a direct boolean test (e.g., replace "if evaluation == True:" with "if
    evaluation:") so the code reads idiomatically and avoids unnecessary
    comparison—locate the check around the evaluation variable usage in the
    function/method handling the workflow and change it accordingly.
    
    app/core/agents/enpkg/tool_smiles.py (1)

    3-15: Duplicate import of BaseTool.

    from langchain.tools import BaseTool appears twice (Line 3 and Line 14), with the second one interleaved between the logger setup. Harmless, but worth tidying while you're touching imports in this file.

    ♻️ Proposed cleanup
     import requests
    
     from langchain.tools import BaseTool
     from pydantic import BaseModel, Field
    
     from typing import Optional
    
     from langchain.callbacks.manager import (
         CallbackManagerForToolRun,
     )
    
     from app.core.session import setup_logger
    
    -from langchain.tools import BaseTool
     logger = setup_logger(__name__)
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/agents/enpkg/tool_smiles.py` around lines 3 - 15, There's a
    duplicate import of BaseTool in this module; remove the redundant `from
    langchain.tools import BaseTool` (the later occurrence) and keep a single import
    at the top, ensuring the logger setup (`logger = setup_logger(__name__)`)
    remains after imports; this cleans imports while leaving `BaseTool`,
    `setup_logger`, and `logger` references intact.
    
    app/core/agents/enpkg/tool_chemicals.py (1)

    6-21: Redundant typing imports.

    Optional is imported three times (Lines 6, 8, 21) and Lines 6/8 duplicate each other. Not a bug, but easy to consolidate while the file is being touched.

    ♻️ Suggested consolidation
    -from typing import List, Optional
    -
    -from typing import Any, Dict, Optional
    +from typing import Any, Dict, List, Optional
     from urllib.error import HTTPError, URLError
     ...
     from pydantic import BaseModel, Field
     from langchain.tools import BaseTool
    -from typing import Optional
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/agents/enpkg/tool_chemicals.py` around lines 6 - 21, The top of
    tool_chemicals.py has duplicated and redundant typing imports (multiple "from
    typing import ..." and repeated Optional); consolidate them into a single import
    statement such as "from typing import List, Optional, Any, Dict" and remove the
    extra "from typing import Any, Dict, Optional" and the later duplicate "from
    typing import Optional" so only one unified typing import remains; update any
    existing symbol references if needed (e.g., usages of List, Optional, Any, Dict)
    to rely on that single import.
    
    app/core/agents/enpkg/tool_taxon.py (1)

    1-8: Nit: duplicate Optional import.

    After adding Optional to the typing import on line 1, the standalone from typing import Optional on line 8 is now redundant. Safe to drop for tidiness.

    Proposed cleanup
     from typing import ClassVar, Optional
    
     from SPARQLWrapper import JSON, SPARQLWrapper
    
    
     from pydantic import BaseModel, Field
    
    -from typing import Optional
    -
     from langchain.callbacks.manager import (
         CallbackManagerForToolRun,
     )
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/agents/enpkg/tool_taxon.py` around lines 1 - 8, The file imports
    Optional twice; remove the redundant standalone "from typing import Optional"
    import (leave the existing "from typing import ClassVar, Optional" import) so
    only one Optional import remains—this cleans up the top-level imports in
    tool_taxon.py without changing any symbols or behavior.
    
    app/core/tests/evaluation.py (1)

    42-77: Module-import side effect writes to the user's LangSmith workspace.

    The dataset existence check and creation (lines 42-77) execute at import time. Any tool that imports app.core.tests.evaluation (linters, test discovery, IDE import-scanning, other modules) will contact LangSmith and potentially create a test_metabot dataset in the user's workspace. Consider wrapping the top-level evaluation logic in an if __name__ == "__main__": guard (or a main() function) so importing the module is a no-op.

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/tests/evaluation.py` around lines 42 - 77, The top-level dataset
    check/creation using dataset_name, client.read_dataset, client.create_dataset
    and client.create_examples runs at import time; move this logic into a function
    (e.g., main()) and protect invocation with an if __name__ == "__main__": guard
    so importing app.core.tests.evaluation is a no-op — relocate all code that reads
    local_data_path, parses the CSV (pd.read_csv), builds inputs/outputs and calls
    client.create_examples into that function and only call it under the guard.
    
    app/core/memory/database_manager.py (1)

    11-20: Minor: unreachable inner guard and implicit None return path.

    In both tools_database() and memory_database(), the outer if already requires both env vars to be truthy, so the inner if class_path: is always true and the else branch only covers the case where either env var is missing. If only DATABASE_URL is set (without the manager class), you silently fall through to the else, which happens to do the right thing today but is accidental. Also, because the inner block has no else, the function technically has an implicit-None return path that a type checker will flag.

    ♻️ Small readability cleanup
     def tools_database():
         ...
    -    if os.getenv('DATABASE_URL') and os.getenv("TOOLS_DATABASE_MANAGER_CLASS"):
    -        class_path = os.getenv('TOOLS_DATABASE_MANAGER_CLASS')
    -        if class_path:
    -            # Dynamically import the module and class based on the environment variable
    -            module_name, class_name = class_path.rsplit('.', 1)
    -            module = importlib.import_module(module_name)
    -            manager_class = getattr(module, class_name)
    -            return manager_class()
    -    else:
    -        return SqliteToolsDatabaseManager()
    +    class_path = os.getenv("TOOLS_DATABASE_MANAGER_CLASS")
    +    if os.getenv("DATABASE_URL") and class_path:
    +        module_name, class_name = class_path.rsplit(".", 1)
    +        module = importlib.import_module(module_name)
    +        manager_class = getattr(module, class_name)
    +        return manager_class()
    +    return SqliteToolsDatabaseManager()

    Apply the same shape to memory_database().

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/memory/database_manager.py` around lines 11 - 20, The nested env-var
    check causes an unreachable inner guard and an implicit None return; refactor
    tools_database() and memory_database() to first read class_path =
    os.getenv("TOOLS_DATABASE_MANAGER_CLASS") and if class_path import
    module_name/class_name and return manager_class() (use importlib.import_module
    and getattr as before), otherwise explicitly return
    SqliteToolsDatabaseManager(); this removes the redundant outer combined if,
    ensures all code paths return a value, and mirrors the same shape in both
    functions.
    
    app/core/memory/custom_sqlite_file.py (1)

    9-23: ClassVar[str] annotation is correct, but SqliteCheckpointerSaver is now unused.

    The ClassVar[str] annotation on database_path is the right fix under Pydantic v2 / langchain_core 0.3.x — without it, a typed default on a Pydantic BaseModel subclass would be interpreted as a model field rather than a class-level constant.

    However, memory_database() in app/core/memory/database_manager.py now returns MemorySaver() by default, making this entire class unreferenced in the actual codebase. The documentation in docs/api-reference/core.md still lists it, but that's stale.

    Consider either:

    • deleting custom_sqlite_file.py along with updating the documentation, or
    • restoring SqliteCheckpointerSaver as the default in memory_database() if disk persistence is required.

    Additionally, the get/put methods here implement the legacy BaseCheckpointSaver API; newer langgraph (0.3.x) uses a different signature, so if you do intend to resurrect this class it will need an API refresh before use.

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/memory/custom_sqlite_file.py` around lines 9 - 23,
    SqliteCheckpointerSaver is now unused because memory_database() returns
    MemorySaver by default; either remove this file and update docs to remove
    SqliteCheckpointerSaver from docs/api-reference/core.md, or restore it as the
    default in memory_database() (replace MemorySaver with SqliteCheckpointerSaver)
    and update its API to match the new langgraph 0.3.x BaseCheckpointSaver
    signature (replace legacy get/put implementations with the new method
    names/signatures). Specifically reference the class SqliteCheckpointerSaver and
    its class var database_path, the factory function memory_database(), the current
    MemorySaver, and the legacy BaseCheckpointSaver/get/put methods when making the
    change.
    
    🤖 Prompt for all review comments with AI agents
    Verify each finding against the current code and only fix it if needed.
    
    Inline comments:
    In `@app/core/agents/sparql/tool_sparql.py`:
    - Line 13: The review notes LLMChain usage is deprecated: replace creation and
    invocation of LLMChain with the LangChain Expression Language (LCEL) pipe
    operator; instead of constructing an LLMChain and calling .run() (references:
    LLMChain and the .run() calls), compose the chain as prompt | llm | parser (or
    prompt | llm if no parser) and call chain.invoke(input) at the two invocation
    sites currently using .run(); update any imports if necessary to use LCEL
    constructs and adjust variable names (e.g., replace existing chain variable
    built from LLMChain) so runtime warnings are removed and the module is
    future-proofed.
    
    In `@app/core/memory/database_manager.py`:
    - Around line 37-40: The default behavior now loses persistence because
    memory_database() returns langgraph.checkpoint.memory.MemorySaver when neither
    DATABASE_URL nor MEMORY_DATABASE_MANAGER_CLASS is set; change memory_database()
    to return your persistent SqliteCheckpointerSaver (from
    app.core.memory.custom_sqlite_file.SqliteCheckpointerSaver) as the default to
    restore prior on-disk checkpoints, keeping MEMORY_DATABASE_MANAGER_CLASS and
    DATABASE_URL overrides intact; also remove the redundant inner `if class_path:`
    guard (dead code) so the outer condition is the single check for a custom class.
    
    In `@app/core/tests/evaluation.py`:
    - Around line 118-128: The evaluate_result function currently assumes either
    "question" or messages[0]["content"] exists and will crash on missing/malformed
    input; update evaluate_result to defensively validate _input: check for a
    non-empty "messages" list, ensure messages[0] is a mapping with a "content"
    string, and only fall back to "question" if present and a string, otherwise
    return a safe error/empty response instead of calling app.invoke; reference the
    input_text construction and the call to app.invoke(message, {"configurable":
    {"thread_id": thread_id}}) so you replace the direct extraction with guarded
    extraction and early-return/normalized message when validation fails.
    - Around line 22-29: The API key validation uses api_key =
    os.getenv("LANGCHAIN_API_KEY") or os.environ.get("LANGSMITH_API_KEY") but raises
    a misleading ValueError mentioning only LANGCHAIN_API_KEY; update the check to
    raise a clear message that reflects both accepted env vars (LANGCHAIN_API_KEY or
    LANGSMITH_API_KEY) so users who set LANGSMITH_API_KEY get accurate
    guidance—modify the ValueError thrown after checking api_key (and keep the
    OPENAI_API_KEY check for openai_key) to reference both environment variable
    names.
    - Around line 44-77: Replace the broad except around client.read_dataset with a
    specific LangSmithNotFoundError from langsmith.utils so only a missing-dataset
    triggers creating from CSV (i.e., change the except Exception: after
    client.read_dataset(...) to except LangSmithNotFoundError: and import that
    symbol); when raising FileNotFoundError for missing local_data_path, use raise
    FileNotFoundError(...) from None to avoid implicit chaining; and broaden the
    inner row-parsing handler in the loop (the try block that json.loads
    row["messages"] / row["__end__"]) to catch both json.JSONDecodeError and
    KeyError (use except (json.JSONDecodeError, KeyError) as e:) so missing CSV
    columns are handled with the existing logger.warning and continue.
    
    In `@app/tests/installation_test.py`:
    - Around line 1-7: Move the sys.path bootstrapping so it runs before importing
    the package: ensure the REPO_ROOT calculation and insertion into sys.path (the
    REPO_ROOT and sys.path.insert logic) execute before the import statement that
    brings in main (from app.core.main import main), so adjust the file to compute
    REPO_ROOT and modify sys.path first and only then import main.
    
    ---
    
    Outside diff comments:
    In `@app/core/tests/evaluation.py`:
    - Around line 1-146: The PR introduces functional changes in this test file (new
    env var setup, LangSmith dataset auto-provisioning, input-extraction logic,
    changed dataset_name) but the PR description claims only dependency updates;
    either update the PR title/description to clearly list these code changes
    (mentioning dataset_name, the env var assignments, dataset creation flow, and
    evaluate_result/create_workflow/run_on_dataset usage) or revert/split those
    changes into a separate PR focused on application logic so the dependency-only
    PR remains limited to manifest updates.
    
    In `@app/core/workflow/langraph_workflow.py`:
    - Line 236: The function process_workflow is annotated with NoReturn but it
    returns implicitly (or after handling exceptions); change its return type
    annotation from NoReturn to None in the process_workflow signature so type
    checkers and readers understand it can complete normally; update the def
    process_workflow(app: StateGraph, question: str, thread_id: int = 1) -> None
    signature accordingly.
    - Around line 90-95: The code currently uses pickle.load on graph_path which can
    execute arbitrary code; modify the loader in langraph_workflow to verify
    integrity before unpickling (e.g., store and check an HMAC signature alongside
    the cached file using a secret key) or replace the cache format with a safe
    serializer (e.g., JSON or msgpack) and deserialize without pickle; specifically
    update the block that reads graph_path and creates graph (the pickle.load call
    that returns graph) to first validate the file signature or switch to a
    non-pickle read/parse routine so untrusted pickle data is never directly passed
    to pickle.load.
    - Line 111: The parameter declaration uses invalid syntax `evaluation=bool`;
    change it to a proper type annotation like `evaluation: bool` and, if a default
    is intended, provide one (e.g. `evaluation: bool = False`) in the
    function/method signature where `evaluation` appears (look for the function in
    langraph_workflow.py that contains the `evaluation` parameter).
    
    ---
    
    Nitpick comments:
    In `@app/core/agents/enpkg/tool_chemicals.py`:
    - Around line 6-21: The top of tool_chemicals.py has duplicated and redundant
    typing imports (multiple "from typing import ..." and repeated Optional);
    consolidate them into a single import statement such as "from typing import
    List, Optional, Any, Dict" and remove the extra "from typing import Any, Dict,
    Optional" and the later duplicate "from typing import Optional" so only one
    unified typing import remains; update any existing symbol references if needed
    (e.g., usages of List, Optional, Any, Dict) to rely on that single import.
    
    In `@app/core/agents/enpkg/tool_smiles.py`:
    - Around line 3-15: There's a duplicate import of BaseTool in this module;
    remove the redundant `from langchain.tools import BaseTool` (the later
    occurrence) and keep a single import at the top, ensuring the logger setup
    (`logger = setup_logger(__name__)`) remains after imports; this cleans imports
    while leaving `BaseTool`, `setup_logger`, and `logger` references intact.
    
    In `@app/core/agents/enpkg/tool_taxon.py`:
    - Around line 1-8: The file imports Optional twice; remove the redundant
    standalone "from typing import Optional" import (leave the existing "from typing
    import ClassVar, Optional" import) so only one Optional import remains—this
    cleans up the top-level imports in tool_taxon.py without changing any symbols or
    behavior.
    
    In `@app/core/memory/custom_sqlite_file.py`:
    - Around line 9-23: SqliteCheckpointerSaver is now unused because
    memory_database() returns MemorySaver by default; either remove this file and
    update docs to remove SqliteCheckpointerSaver from docs/api-reference/core.md,
    or restore it as the default in memory_database() (replace MemorySaver with
    SqliteCheckpointerSaver) and update its API to match the new langgraph 0.3.x
    BaseCheckpointSaver signature (replace legacy get/put implementations with the
    new method names/signatures). Specifically reference the class
    SqliteCheckpointerSaver and its class var database_path, the factory function
    memory_database(), the current MemorySaver, and the legacy
    BaseCheckpointSaver/get/put methods when making the change.
    
    In `@app/core/memory/database_manager.py`:
    - Around line 11-20: The nested env-var check causes an unreachable inner guard
    and an implicit None return; refactor tools_database() and memory_database() to
    first read class_path = os.getenv("TOOLS_DATABASE_MANAGER_CLASS") and if
    class_path import module_name/class_name and return manager_class() (use
    importlib.import_module and getattr as before), otherwise explicitly return
    SqliteToolsDatabaseManager(); this removes the redundant outer combined if,
    ensures all code paths return a value, and mirrors the same shape in both
    functions.
    
    In `@app/core/tests/evaluation.py`:
    - Around line 42-77: The top-level dataset check/creation using dataset_name,
    client.read_dataset, client.create_dataset and client.create_examples runs at
    import time; move this logic into a function (e.g., main()) and protect
    invocation with an if __name__ == "__main__": guard so importing
    app.core.tests.evaluation is a no-op — relocate all code that reads
    local_data_path, parses the CSV (pd.read_csv), builds inputs/outputs and calls
    client.create_examples into that function and only call it under the guard.
    
    In `@app/core/workflow/langraph_workflow.py`:
    - Line 204: The condition "if evaluation == True:" is non-idiomatic; update the
    check in langraph_workflow.py to use a direct boolean test (e.g., replace "if
    evaluation == True:" with "if evaluation:") so the code reads idiomatically and
    avoids unnecessary comparison—locate the check around the evaluation variable
    usage in the function/method handling the workflow and change it accordingly.
    
    🪄 Autofix (Beta)

    Fix all unresolved CodeRabbit comments on this PR:

    • Push a commit to this branch (recommended)
    • Create a new PR with the fixes

    ℹ️ Review info
    ⚙️ Run configuration

    Configuration used: Repository UI

    Review profile: CHILL

    Plan: Pro

    Run ID: 6b167ef3-7593-423b-968c-2df542b85129

    📥 Commits

    Reviewing files that changed from the base of the PR and between 92dddc8 and 085c973.

    ⛔ Files ignored due to path filters (3)
    • app/data/big_benchmark.csv is excluded by !**/*.csv, !**/*.csv
    • app/data/evaluation_dataset.csv is excluded by !**/*.csv, !**/*.csv
    • environment.yml is excluded by !**/*.yml
    📒 Files selected for processing (17)
    • app/core/agents/enpkg/tool_chemicals.py
    • app/core/agents/enpkg/tool_smiles.py
    • app/core/agents/enpkg/tool_target.py
    • app/core/agents/enpkg/tool_taxon.py
    • app/core/agents/entry/tool_filesparser.py
    • app/core/agents/interpreter/tool_interpreter.py
    • app/core/agents/interpreter/tool_spectrum.py
    • app/core/agents/sparql/tool_merge_result.py
    • app/core/agents/sparql/tool_sparql.py
    • app/core/agents/sparql/tool_wikidata_query.py
    • app/core/agents/validator/tool_validator.py
    • app/core/memory/custom_sqlite_file.py
    • app/core/memory/database_manager.py
    • app/core/tests/evaluation.py
    • app/core/workflow/langraph_workflow.py
    • app/tests/installation_test.py
    • requirements.txt

    from langchain.chains.llm import LLMChain
    from langchain_core.prompts.prompt import PromptTemplate
    from langchain.pydantic_v1 import BaseModel, Field
    from pydantic import BaseModel, Field

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue | 🟡 Minor

    🧩 Analysis chain

    🌐 Web query:

    Is langchain.chains.LLMChain deprecated in langchain 0.3, and what is the recommended replacement?

    💡 Result:

    Yes, langchain.chains.LLMChain is deprecated in LangChain 0.3 (deprecated since version 0.1.17, available in langchain-classic or legacy paths in 0.3). The recommended replacement is LangChain Expression Language (LCEL) using the pipe operator (|): chain = prompt | llm | parser, then chain.invoke(input). Example migration: from langchain_core.prompts import PromptTemplate from langchain_core.output_parsers import StrOutputParser from langchain_openai import OpenAI prompt = PromptTemplate.from_template("Tell me a {adjective} joke about {topic}") model = OpenAI chain = prompt | model | StrOutputParser result = chain.invoke({"adjective": "funny", "topic": "cats"})

    Citations:


    🏁 Script executed:

    # First, find and examine the sparql tool file
    fd -t f "tool_sparql.py" | head -5

    Repository: HolobiomicsLab/MetaboT

    Length of output: 104


    🏁 Script executed:

    # Get file size first to see if we can read it all
    wc -l app/core/agents/sparql/tool_sparql.py

    Repository: HolobiomicsLab/MetaboT

    Length of output: 108


    🏁 Script executed:

    # Read the relevant sections of the file
    cat -n app/core/agents/sparql/tool_sparql.py | sed -n '1,20p'

    Repository: HolobiomicsLab/MetaboT

    Length of output: 789


    🏁 Script executed:

    # Check lines around the imports and field definitions
    cat -n app/core/agents/sparql/tool_sparql.py | sed -n '200,240p'

    Repository: HolobiomicsLab/MetaboT

    Length of output: 2074


    🏁 Script executed:

    # Check lines with LLMChain usage
    cat -n app/core/agents/sparql/tool_sparql.py | sed -n '260,300p'

    Repository: HolobiomicsLab/MetaboT

    Length of output: 1948


    🏁 Script executed:

    # Check lines around 240-250 for the second LLMChain instantiation
    cat -n app/core/agents/sparql/tool_sparql.py | sed -n '238,252p'

    Repository: HolobiomicsLab/MetaboT

    Length of output: 720


    LGTM on the Pydantic migration; note LLMChain deprecation with langchain 0.3.27.

    The import switch and typed class attributes (lines 210–211, 229, 232) are correct and consistent with other tool modules.

    Separately (out of scope for this PR, but worth tracking): LLMChain has been deprecated since langchain 0.1.17 and remains deprecated in 0.3.x. Calls to .run() at lines 267 and 289 will emit LangChainDeprecationWarnings at runtime. The recommended migration path is LangChain Expression Language (LCEL) using the pipe operator: chain = prompt | llm | parser, then chain.invoke(input). This would future-proof the module ahead of langchain 1.0, where LLMChain is slated for removal.

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/agents/sparql/tool_sparql.py` at line 13, The review notes LLMChain
    usage is deprecated: replace creation and invocation of LLMChain with the
    LangChain Expression Language (LCEL) pipe operator; instead of constructing an
    LLMChain and calling .run() (references: LLMChain and the .run() calls), compose
    the chain as prompt | llm | parser (or prompt | llm if no parser) and call
    chain.invoke(input) at the two invocation sites currently using .run(); update
    any imports if necessary to use LCEL constructs and adjust variable names (e.g.,
    replace existing chain variable built from LLMChain) so runtime warnings are
    removed and the module is future-proofed.
    

    Comment on lines +37 to +40
    else:
    # Use langgraph's built-in MemorySaver which implements the current checkpoint API
    from langgraph.checkpoint.memory import MemorySaver
    return MemorySaver() No newline at end of file

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue | 🟠 Major

    🧩 Analysis chain

    🌐 Web query:

    In langgraph 0.3.34, what is the import path for the official SQLite checkpointer and does MemorySaver persist state to disk?

    💡 Result:

    In langgraph 0.3.34, the official SQLite checkpointer is SqliteSaver (or AsyncSqliteSaver), imported from langgraph.checkpoint.sqlite after installing the separate langgraph-checkpoint-sqlite package. MemorySaver does not persist state to disk; it stores state in RAM (in-process memory) only and loses data on process restart.

    Citations:


    🏁 Script executed:

    # Search for SqliteCheckpointerSaver definition
    fd -t f "custom_sqlite_file" --case-sensitive

    Repository: HolobiomicsLab/MetaboT

    Length of output: 104


    🏁 Script executed:

    # Search for SqliteCheckpointerSaver usage
    rg "SqliteCheckpointerSaver" --type py

    Repository: HolobiomicsLab/MetaboT

    Length of output: 157


    🏁 Script executed:

    # Verify the two caller locations mentioned
    rg "streamlit_app\.py|langraph_workflow\.py" --type py --files-with-matches

    Repository: HolobiomicsLab/MetaboT

    Length of output: 48


    🏁 Script executed:

    # Check the memory_database function location and current content
    cat -n app/core/memory/database_manager.py | head -50

    Repository: HolobiomicsLab/MetaboT

    Length of output: 1900


    🏁 Script executed:

    rg "memory_database\(\)" --type py -B 2 -A 2

    Repository: HolobiomicsLab/MetaboT

    Length of output: 986


    🏁 Script executed:

    # Search for streamlit files
    fd -t f "streamlit" --case-sensitive

    Repository: HolobiomicsLab/MetaboT

    Length of output: 136


    🏁 Script executed:

    # Search for langgraph workflow files
    fd -t f ".*workflow" --case-sensitive -e py

    Repository: HolobiomicsLab/MetaboT

    Length of output: 105


    🏁 Script executed:

    # Check for memory usage in app code
    rg "from app.core.memory" --type py

    Repository: HolobiomicsLab/MetaboT

    Length of output: 1020


    🏁 Script executed:

    git log --oneline -20 app/core/memory/database_manager.py

    Repository: HolobiomicsLab/MetaboT

    Length of output: 129


    🏁 Script executed:

    git show HEAD:app/core/memory/database_manager.py | head -50

    Repository: HolobiomicsLab/MetaboT

    Length of output: 1620


    🏁 Script executed:

    # Check the conditional logic more carefully
    cat -n app/core/memory/database_manager.py | sed -n '22,40p'

    Repository: HolobiomicsLab/MetaboT

    Length of output: 996


    🏁 Script executed:

    # Check if there's a recent commit changing memory_database
    git log -p --all -S "MemorySaver" -- app/core/memory/database_manager.py | head -100

    Repository: HolobiomicsLab/MetaboT

    Length of output: 48


    🏁 Script executed:

    cat -n app/core/memory/custom_sqlite_file.py | head -80

    Repository: HolobiomicsLab/MetaboT

    Length of output: 3523


    🏁 Script executed:

    # Check imports and BaseCheckpointSaver compatibility
    rg "BaseCheckpointSaver|langgraph" app/core/memory/custom_sqlite_file.py -B 2 -A 2

    Repository: HolobiomicsLab/MetaboT

    Length of output: 909


    Behavioral regression: default memory is no longer persisted across restarts.

    Previously, when neither DATABASE_URL nor MEMORY_DATABASE_MANAGER_CLASS were set, memory_database() returned a SqliteCheckpointerSaver that persisted checkpoints to langgraph_checkpoint.db on disk. MemorySaver stores state in RAM only — all conversation history is lost when the process exits.

    Impact on the two callers:

    • streamlit_webapp/streamlit_app.py stores the saver in st.session_state.memory. st.session_state only persists across Streamlit reruns within a single browser session, not across app restarts / redeploys / container recycles. Under the previous default, checkpoints survived restarts via SQLite; under this change they do not.
    • app/core/workflow/langraph_workflow.py passes this to workflow.compile(checkpointer=memory), so the same loss applies to the core workflow.

    This is a user-visible behavior change that contradicts the PR description's "no application code changes" claim. Two reasonable options:

    ♻️ Option A — restore SQLite persistence as default (preserves prior behavior)
         else:
    -        # Use langgraph's built-in MemorySaver which implements the current checkpoint API
    -        from langgraph.checkpoint.memory import MemorySaver
    -        return MemorySaver()
    +        return SqliteCheckpointerSaver()

    SqliteCheckpointerSaver is already defined in app/core/memory/custom_sqlite_file.py and implements BaseCheckpointSaver.

    ♻️ Option B — migrate to langgraph's official SQLite saver
         else:
    -        # Use langgraph's built-in MemorySaver which implements the current checkpoint API
    -        from langgraph.checkpoint.memory import MemorySaver
    -        return MemorySaver()
    +        from langgraph.checkpoint.sqlite import SqliteSaver
    +        return SqliteSaver.from_conn_string("langgraph_checkpoint.db")

    Requires adding langgraph-checkpoint-sqlite to environment.yml / requirements.txt.

    Minor: Lines 30–31 have unreachable dead code — the if class_path: guard is redundant given the outer and condition on line 29.

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/memory/database_manager.py` around lines 37 - 40, The default
    behavior now loses persistence because memory_database() returns
    langgraph.checkpoint.memory.MemorySaver when neither DATABASE_URL nor
    MEMORY_DATABASE_MANAGER_CLASS is set; change memory_database() to return your
    persistent SqliteCheckpointerSaver (from
    app.core.memory.custom_sqlite_file.SqliteCheckpointerSaver) as the default to
    restore prior on-disk checkpoints, keeping MEMORY_DATABASE_MANAGER_CLASS and
    DATABASE_URL overrides intact; also remove the redundant inner `if class_path:`
    guard (dead code) so the outer condition is the single check for a custom class.
    

    Comment on lines +22 to +29
    # 2. Check for required API keys safely
    api_key = os.getenv("LANGCHAIN_API_KEY") or os.environ.get("LANGSMITH_API_KEY")
    openai_key = os.getenv("OPENAI_API_KEY")

    if not api_key:
    raise ValueError("The environment variable LANGCHAIN_API_KEY is not defined")
    raise ValueError("Missing LANGCHAIN_API_KEY. Please copy .env.template to .env and add your key.")
    if not openai_key:
    raise ValueError("Missing OPENAI_API_KEY. Please copy .env.template to .env and add your key.")

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue | 🟡 Minor

    Inconsistent error messages for API key validation.

    Line 23 accepts LANGCHAIN_API_KEY or LANGSMITH_API_KEY, but the error message at line 27 only mentions LANGCHAIN_API_KEY. Users who set LANGSMITH_API_KEY won't hit this path, but users with neither will see a misleading message.

    Proposed fix
     if not api_key:
    -    raise ValueError("Missing LANGCHAIN_API_KEY. Please copy .env.template to .env and add your key.")
    +    raise ValueError("Missing LANGCHAIN_API_KEY (or LANGSMITH_API_KEY). Please copy .env.template to .env and add your key.")
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/tests/evaluation.py` around lines 22 - 29, The API key validation
    uses api_key = os.getenv("LANGCHAIN_API_KEY") or
    os.environ.get("LANGSMITH_API_KEY") but raises a misleading ValueError
    mentioning only LANGCHAIN_API_KEY; update the check to raise a clear message
    that reflects both accepted env vars (LANGCHAIN_API_KEY or LANGSMITH_API_KEY) so
    users who set LANGSMITH_API_KEY get accurate guidance—modify the ValueError
    thrown after checking api_key (and keep the OPENAI_API_KEY check for openai_key)
    to reference both environment variable names.
    

    Comment on lines +44 to +77
    try:
    client.read_dataset(dataset_name=dataset_name)
    logger.info(f"Dataset '{dataset_name}' found in LangSmith workspace.")
    except Exception:
    logger.info(f"Dataset '{dataset_name}' not found. Attempting to create it from local file...")


    if not os.path.exists(local_data_path):
    raise FileNotFoundError(
    f"Could not find '{local_data_path}'. Ensure the local dataset file is shared alongside this script.")


    # Load local data and create the dataset in the user's LangSmith account
    df = pd.read_csv(local_data_path)
    dataset = client.create_dataset(dataset_name=dataset_name, description="MetaboT Benchmark")

    inputs = []
    outputs = []

    for _, row in df.iterrows():
    try:
    # The CSV stores the JSON arrays as strings, so we need to parse them back into Python objects
    parsed_messages = json.loads(row["messages"])
    parsed_end_state = json.loads(row["__end__"])

    inputs.append({"messages": parsed_messages})
    outputs.append({"__end__": parsed_end_state})

    except json.JSONDecodeError as e:
    logger.warning(f"Skipping a row due to JSON parsing error: {e}")
    continue

    client.create_examples(inputs=inputs, outputs=outputs, dataset_id=dataset.id)
    logger.info(f"Successfully created dataset '{dataset_name}' in LangSmith.")

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue | 🟠 Major

    🧩 Analysis chain

    🌐 Web query:

    Which exception class does langsmith.Client.read_dataset raise when the dataset does not exist in langsmith 0.3.45?

    💡 Result:

    langsmith.utils.LangSmithNotFoundError

    Citations:


    🏁 Script executed:

    cat -n app/core/tests/evaluation.py | sed -n '40,80p'

    Repository: HolobiomicsLab/MetaboT

    Length of output: 1972


    🏁 Script executed:

    head -30 app/core/tests/evaluation.py

    Repository: HolobiomicsLab/MetaboT

    Length of output: 1053


    Narrow except Exception to catch only "dataset not found" errors; handle missing CSV columns in inner exception handler.

    Catching bare Exception on line 47 masks auth failures, network errors, rate-limit errors, and other genuine LangSmith SDK errors by treating them identically to "dataset not found" — silently (re)creating the dataset from the local CSV. This can produce duplicate datasets or hide actual outages. Replace with except LangSmithNotFoundError (imported from langsmith.utils). When raising FileNotFoundError inside the except block (lines 52–53), use raise ... from None to avoid implicit exception chaining (ruff B904).

    Additionally, the inner try/except (line 72) only catches json.JSONDecodeError. If the CSV is missing the messages or __end__ columns, a KeyError will propagate out of the loop instead of triggering the warning/skip behavior. Catch both: except (json.JSONDecodeError, KeyError) as e:.

    Proposed fix sketch
    -from langsmith import Client
    +from langsmith import Client
    +from langsmith.utils import LangSmithNotFoundError
    @@
    -try:
    -    client.read_dataset(dataset_name=dataset_name)
    -    logger.info(f"Dataset '{dataset_name}' found in LangSmith workspace.")
    -except Exception:
    -    logger.info(f"Dataset '{dataset_name}' not found. Attempting to create it from local file...")
    -
    -
    -    if not os.path.exists(local_data_path):
    -        raise FileNotFoundError(
    -            f"Could not find '{local_data_path}'. Ensure the local dataset file is shared alongside this script.")
    +try:
    +    client.read_dataset(dataset_name=dataset_name)
    +    logger.info(f"Dataset '{dataset_name}' found in LangSmith workspace.")
    +except LangSmithNotFoundError:
    +    logger.info(f"Dataset '{dataset_name}' not found. Attempting to create it from local file...")
    +
    +    if not os.path.exists(local_data_path):
    +        raise FileNotFoundError(
    +            f"Could not find '{local_data_path}'. Ensure the local dataset file is shared alongside this script."
    +        ) from None
    @@
    -        except json.JSONDecodeError as e:
    +        except (json.JSONDecodeError, KeyError) as e:
                 logger.warning(f"Skipping a row due to JSON parsing error: {e}")
                 continue
    🧰 Tools
    🪛 Ruff (0.15.11)

    [warning] 47-47: Do not catch blind exception: Exception

    (BLE001)


    [warning] 52-53: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling

    (B904)

    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/tests/evaluation.py` around lines 44 - 77, Replace the broad except
    around client.read_dataset with a specific LangSmithNotFoundError from
    langsmith.utils so only a missing-dataset triggers creating from CSV (i.e.,
    change the except Exception: after client.read_dataset(...) to except
    LangSmithNotFoundError: and import that symbol); when raising FileNotFoundError
    for missing local_data_path, use raise FileNotFoundError(...) from None to avoid
    implicit chaining; and broaden the inner row-parsing handler in the loop (the
    try block that json.loads row["messages"] / row["__end__"]) to catch both
    json.JSONDecodeError and KeyError (use except (json.JSONDecodeError, KeyError)
    as e:) so missing CSV columns are handled with the existing logger.warning and
    continue.
    

    Comment on lines 118 to 128
    def evaluate_result(_input, thread_id: int = 1):
    """
    Evaluate the result based on input.

    Args:
    _input (dict): Input containing messages to process

    Returns:
    dict: The evaluation result
    """

    # Prepare the message
    """Evaluate the result based on input."""
    # Note: Adjust the 'question' key below if your dataset uses a different input key
    input_text = _input.get("question") or _input["messages"][0]["content"]

    message = {
    "messages": [
    HumanMessage(content=_input["messages"][0]["content"])
    ]
    "messages": [HumanMessage(content=input_text)]
    }


    response = app.invoke(message,
    {"configurable": {"thread_id": thread_id}},)

    response = app.invoke(message, {"configurable": {"thread_id": thread_id}})
    return {"output": response}

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue | 🟡 Minor

    evaluate_result will crash on malformed/empty input.

    _input.get("question") or _input["messages"][0]["content"] will raise KeyError/IndexError if messages is absent or empty, and TypeError if the first element isn't a dict with content. Given the auto-created dataset writes {"messages": [...]} (no "question" key), the fallback branch is the primary path — so any malformed row that slipped past the ingestion loop will crash the evaluator.

    Proposed defensive fix
     def evaluate_result(_input, thread_id: int = 1):
         """Evaluate the result based on input."""
         # Note: Adjust the 'question' key below if your dataset uses a different input key
    -    input_text = _input.get("question") or _input["messages"][0]["content"]
    +    input_text = _input.get("question")
    +    if not input_text:
    +        messages = _input.get("messages") or []
    +        if not messages or not isinstance(messages[0], dict) or "content" not in messages[0]:
    +            raise ValueError(f"Example has no 'question' and no usable 'messages[0].content': {_input!r}")
    +        input_text = messages[0]["content"]
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/core/tests/evaluation.py` around lines 118 - 128, The evaluate_result
    function currently assumes either "question" or messages[0]["content"] exists
    and will crash on missing/malformed input; update evaluate_result to defensively
    validate _input: check for a non-empty "messages" list, ensure messages[0] is a
    mapping with a "content" string, and only fall back to "question" if present and
    a string, otherwise return a safe error/empty response instead of calling
    app.invoke; reference the input_text construction and the call to
    app.invoke(message, {"configurable": {"thread_id": thread_id}}) so you replace
    the direct extraction with guarded extraction and early-return/normalized
    message when validation fails.
    

    Comment on lines 1 to +7
    import sys
    from pathlib import Path
    from app.core.main import main

    REPO_ROOT = Path(__file__).resolve().parents[2]
    if str(REPO_ROOT) not in sys.path:
    sys.path.insert(0, str(REPO_ROOT))

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    ⚠️ Potential issue | 🔴 Critical

    Import order defeats the new sys.path bootstrap.

    from app.core.main import main on line 3 runs before the REPO_ROOT insertion on lines 5-7. When the test is launched from a directory where the repo root isn’t already on sys.path (the exact scenario this bootstrap is meant to handle), the import fails with ModuleNotFoundError: No module named 'app' and the path-fix code never executes. The sys.path adjustment must precede the project import.

    🔧 Proposed fix
     import sys
     from pathlib import Path
    -from app.core.main import main
    -
    -REPO_ROOT = Path(__file__).resolve().parents[2]
    -if str(REPO_ROOT) not in sys.path:
    -    sys.path.insert(0, str(REPO_ROOT))
    +
    +REPO_ROOT = Path(__file__).resolve().parents[2]
    +if str(REPO_ROOT) not in sys.path:
    +    sys.path.insert(0, str(REPO_ROOT))
    +
    +from app.core.main import main  # noqa: E402  (import after sys.path bootstrap)
    🤖 Prompt for AI Agents
    Verify each finding against the current code and only fix it if needed.
    
    In `@app/tests/installation_test.py` around lines 1 - 7, Move the sys.path
    bootstrapping so it runs before importing the package: ensure the REPO_ROOT
    calculation and insertion into sys.path (the REPO_ROOT and sys.path.insert
    logic) execute before the import statement that brings in main (from
    app.core.main import main), so adjust the file to compute REPO_ROOT and modify
    sys.path first and only then import main.
    

    The interpreter tool previously relied on codeinterpreterapi (remote sandbox) which is no longer
    compatible with the current dependency stack. This replaces it with langchain_experimental
    PythonREPL (local execution) while preserving the same agent contract.
    
    Key changes:
    - tool_interpreter.py: rewrite using PythonREPL — injects file previews (columns, dtypes,
      3-row sample) into the LLM prompt so generated code uses exact column names; truncates
      REPL stdout at 2000 chars to prevent context overflow; wires shared llm_instance from
      the agent instead of instantiating a separate model
    - agent.py: passes llm_instance to tool constructor via import_tools introspection
    - params.ini: bump default OpenAI models from gpt-4o / gpt-4o-mini to gpt-5.4 / gpt-5.4-mini
    - requirements.txt / environment.yml: pin langchain_experimental==0.3.4 and remove
      codeinterpreterapi / codeboxapi dependencies
    - Remove custom_sqlite_file.py (unused after langgraph checkpoint refactor)
    
    Security note: PythonREPL executes LLM-generated code in-process without sandboxing.
    A warning comment has been added; restrict to trusted deployment environments.
    @dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 24, 2026
    @lfnothias lfnothias closed this Apr 24, 2026
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    critical Priority task Dependencies Review effort 1/5 size:XL This PR changes 500-999 lines, ignoring generated files.

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants