Harrison/fix logging api #1

recursiveAF · 2024-11-15T13:58:16Z

Thank you for contributing to LangChain!

PR title: "package: description"
- Where "package" is whichever of langchain, community, core, etc. is being modified. Use "docs: ..." for purely docs changes, "infra: ..." for CI changes.
- Example: "community: add foobar LLM"
PR message: Delete this entire checklist and replace with
- Description: a description of the change
- Issue: the issue # it fixes, if applicable
- Dependencies: any dependencies required for this change
- Twitter handle: if your PR gets announced, and you'd like a mention, we'll gladly shout you out!
Add tests and docs: If you're adding a new integration, please include
1. a test for the integration, preferably unit tests that do not rely on network access,
2. an example notebook showing its use. It lives in docs/docs/integrations directory.
Lint and test: Run make format, make lint and make test from the root of the package(s) you've modified. See contribution guidelines for more: https://python.langchain.com/docs/contributing/

Additional guidelines:

Make sure optional dependencies are imported within a function.
Please do not add dependencies to pyproject.toml files (even optional ones) unless they are required for unit tests.
Most PRs should not touch more than one package.
Changes should be backwards compatible.
If you are adding something to community, do not re-import it in langchain.

If no one reviews your PR within a few days, please @-mention one of baskaryan, efriis, eyurtsev, ccurme, vbarda, hwchase17.

recurseml · 2024-11-15T13:59:26Z

langchain/__init__.py

 from langchain.sql_database import SQLDatabase
 from langchain.vectorstores import FAISS, ElasticVectorSearch

+logger = BaseLogger()


The code instantiates a BaseLogger which is likely meant to be an abstract base class. This could lead to missing implementations of required logging methods. Should use a concrete logger implementation instead.

recurseml · 2024-11-15T13:59:27Z

langchain/__init__.py

    VectorDBQA,
    VectorDBQAWithSourcesChain,
 )
 from langchain.docstore import InMemoryDocstore, Wikipedia


Importing Wikipedia directly from langchain.docstore is deprecated. It should be imported from langchain.docstore.wikipedia to follow the package's structure

recurseml · 2024-11-15T13:59:29Z

langchain/agents/agent.py

 from langchain.chains.base import Chain
 from langchain.chains.llm import LLMChain
-from langchain.input import ChainedInput, get_color_mapping
+from langchain.input import get_color_mapping


Incorrect import path. The 'input' module seems incorrect - should likely be importing from a different module like langchain.utils or similar since input handling related utilities are typically not in a top-level 'input' module

recurseml · 2024-11-15T13:59:31Z

langchain/agents/agent.py

            full_output = self._fix_text(full_output)
            inputs = {input_key: text + full_output, "stop": self._stop}
            output = self.llm_chain.predict(**inputs)
            full_output += output


Potential infinite loop risk. If _extract_tool_and_input keeps returning None, this will keep appending output indefinitely. Should add a maximum retry limit or other exit condition

recurseml · 2024-11-15T13:59:33Z

langchain/agents/input.py

+        """Add text to input, print if in verbose mode."""
+        if self._verbose:
+            langchain.logger.log_agent_action(action, color=color)
+        self._input += action.log


Attempting to add strings without first ensuring a newline or proper spacing. This could cause text to run together. Should include proper separators like: self._input += f'\n{action.log}'

recurseml · 2024-11-15T13:59:34Z

langchain/agents/input.py

+            langchain.logger.log_agent_start(text)
+        self._input = text
+
+    def add_action(self, action: AgentAction, color: Optional[str] = None) -> None:


The color parameter is unused in the method implementation. Either remove the unused parameter or implement the color functionality in the method

recurseml · 2024-11-15T13:59:36Z

langchain/chains/api/base.py

        )
        if self.verbose:
            print_text(api_url, color="green", end="\n")
        api_response = self.requests_wrapper.run(api_url)


API response text is processed without checking the response status code. Should verify response.status_code is in the 200-299 range before processing the text

recurseml · 2024-11-15T13:59:38Z

langchain/chains/llm.py

-            print_text(prompt, color="green", end="\n")
+            langchain.logger.log_llm_inputs(selected_inputs, prompt)
        kwargs = {}
        if "stop" in inputs:


The code checks for 'stop' in the main inputs dictionary, but this parameter should be handled separately from prompt inputs. This could lead to 'stop' being passed to the prompt template if it's included in input_variables. Should separate LLM-specific parameters from prompt parameters.

recurseml · 2024-11-15T13:59:40Z

langchain/input.py

    if color is None:
        print(text, end=end)
    else:
        color_str = _TEXT_COLOR_MAPPING[color]


Direct dictionary access without checking if the color exists in _TEXT_COLOR_MAPPING. This could raise a KeyError if an invalid color is provided. Should add a check or use .get() method with a default value

recurseml · 2024-11-15T13:59:42Z

langchain/logger.py

+    def log_agent_observation(self, observation: str, **kwargs: Any):
+        pass
+
+    def log_llm_inputs(self, inputs: dict, prompt: str, **kwargs):


Missing return type annotation -> should be -> None for consistency with other methods in the class and to follow proper typing practices

recurseml · 2024-11-15T13:59:43Z

langchain/logger.py

+    def log_llm_inputs(self, inputs: dict, prompt: str, **kwargs):
+        pass
+
+    def log_llm_response(self, output: str, **kwargs):


Missing return type annotation -> should be -> None for consistency with other methods

recurseml · 2024-11-15T13:59:45Z

langchain/logger.py

+        llm_prefix: Optional[str] = None,
+        **kwargs: Any,
+    ):
+        print_text(f"\n{observation_prefix}")


observation_prefix could be None, which would print 'None'. Should check if observation_prefix exists before printing

recurseml · 2024-11-15T13:59:47Z

langchain/logger.py

+    ):
+        print_text(f"\n{observation_prefix}")
+        print_text(observation, color=color)
+        print_text(f"\n{llm_prefix}")


llm_prefix could be None, which would print 'None'. Should check if llm_prefix exists before printing

recurseml · 2024-11-15T13:59:49Z

langchain/logger.py

+
+
+class BaseLogger:
+    def log_agent_start(self, text: str, **kwargs: Any):


Missing return type annotation -> should be -> None for consistency with base class

recurseml · 2024-11-15T13:59:50Z

langchain/logger.py

+    def log_agent_start(self, text: str, **kwargs: Any):
+        pass
+
+    def log_agent_end(self, text: str, **kwargs: Any):


Missing return type annotation -> should be -> None for consistency with base class

recurseml · 2024-11-15T13:59:52Z

langchain/schema.py

@@ -0,0 +1,11 @@
+from __future__ import annotations


This import is redundant in Python 3.7+ since string annotations are the default. While not strictly a bug, it's unnecessary code that could be removed for cleaner code. Additionally, in Python 3.9+, the typing.NamedTuple already handles forward references automatically.

recurseml · 2024-11-15T13:59:54Z

pyproject.toml

-version = "0.0.28"
+version = "0.0.29"
 description = "Building applications with LLMs through composability"
 authors = []


Empty authors list. While technically valid, it's recommended to include at least one author for proper package metadata.

hwchase17 added 8 commits December 4, 2022 08:45

stash

2bef195

Merge branch 'master' into harrison/logger

7ec2107

stash

c2580cf

Merge branch 'master' into harrison/logger

ad85f3b

wip logging

bf8bed4

fix logging in api chain

ec842b7

Merge branch 'master' into harrison/fix_logging_api

64ea17b

cr

ec65ca0

recurseml bot reviewed Nov 15, 2024

View reviewed changes



		class BaseLogger:
		def log_agent_start(self, text: str, **kwargs: Any):

Harrison/fix logging api #1

Are you sure you want to change the base?

Harrison/fix logging api #1

Uh oh!

Conversation

recursiveAF commented Nov 15, 2024

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

recurseml bot Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants