Skip to content

[CM-8549] added error handling for chains and made raise_exception False by def…#90

Merged
alexkuzmik merged 9 commits intomainfrom
CM-8549_add_error_handling_for_current_chain
Nov 15, 2023
Merged

[CM-8549] added error handling for chains and made raise_exception False by def…#90
alexkuzmik merged 9 commits intomainfrom
CM-8549_add_error_handling_for_current_chain

Conversation

@jynx10
Copy link
Copy Markdown
Contributor

@jynx10 jynx10 commented Nov 7, 2023

…ault

Comment thread src/comet_llm/chains/span.py Outdated

def __api__start__(self, chain: "chain.Chain") -> None:
self._connect_to_chain(chain)
if chain is not None:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's move this if-statement to __enter__ method and not call __api__start__ if chain is None.
__api__start__ explicitly expects not None object, let's keep this certainty here.

Comment thread src/comet_llm/chains/state.py Outdated
return _APP_STATE.chain_exists()


@exceptions.filter(allow_raising=config.raising_enabled(), summary=app.SUMMARY)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was thinking about this decorator.
If we start abusing it everywhere we want it will become a problem soon to understand the exception-related logic in the SDK, especially if it's config dependent.
Let's use this decorator (and in general the logic for deciding between logging and raising) only for the public API level. I think we will benefit from it in the future.

So what I propose here is this:

# inside State class
    @property
    def chain(self) -> Optional["chain.Chain"]:
        result: "chain.Chain" = self._thread_context_registry.get("global-chain")
        return result

def get_global_chain() -> Optional["chain.Chain"]:
    result = _APP_STATE.chain

Let's raise an exception directly from methods like end_chain (which can now be decorated with filter decorator) and from Span class.

Comment thread src/comet_llm/chains/api.py Outdated
"""
global_chain = state.get_global_chain()
if global_chain is None:
return # type: ignore
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

after the changes I proposed we'll have something like this here:

if global_chain is None:
    raise exceptions.CometLLMException(
        "Global chain is not initialized for this thread. Initialize it with `comet_llm.start_chain(...)`"
    )

Comment thread tests/unit/chains/test_chains_api.py Outdated
assert result == llm_result.LLMResult(id="experiment-id", project_url="project-url")


def test_end_chain__no_chain_started__error_logged():
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We don't need such unit test here. Below are some points about it:

  1. end_chain is one piece of logic, exceptions.filter is another one. end_chain's responsibility is to raise an exception if there is no chain started, filter's responsibility is to catch it if it's configured. With this test we can simply remove raising from your code and the test will still pass. For someone without context this test might look confusing.

  2. In order to test that end_chain raises an exception, we can extract the original function from the decorated one like this:

end_chain = end_chain.__wrapped__
  1. In order to check that we have filtering logic enabled for our public API functions, we can create a separate test file for this feature, which will check that all required public functions are covered by filtering. But it's out of the scope of this ticket :)

}


def test_span__no_chain_started_raising_exceptions_disabled__wont_connect_to_chain():
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When there was no chain started, there is also no timer usage. It means that here in this test timer should be something like this

timer = box.Box(duration=None, start_timestamp=None, end_timestamp=None)

@alexkuzmik alexkuzmik merged commit 69dbd7c into main Nov 15, 2023
@jynx10 jynx10 deleted the CM-8549_add_error_handling_for_current_chain branch November 15, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants