langchaingo icon indicating copy to clipboard operation
langchaingo copied to clipboard

llms: extract common llms errors to shared declaration

Open wgeorgecook opened this issue 1 year ago • 7 comments

PR Checklist

  • [x] Read the Contributing documentation.
  • [x] Read the Code of conduct documentation.
  • [x] Name your Pull Request title clearly, concisely, and prefixed with the name of the primarily affected package you changed according to Good commit messages (such as memory: add interfaces for X, Y or util: add whizzbang helpers).
  • [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. Fixes #123).
  • [x] Describes the source of new concepts.
  • [x] References existing implementations as appropriate.
  • [x] Contains test coverage for new functions.
  • [x] Passes all golangci-lint checks.

This addresses the comment Travis made in this PR I previously made. It specifically extracts the ErrorIncompleteEmbedding error and the ErrorEmptyResponse error used in the files in the /llms directory.

I specifically targeted these errors because they're the ones I encountered using LangChain Go, there is still more work to be done in consolidating all of the other potentially shared errors.

wgeorgecook avatar Jun 22 '24 16:06 wgeorgecook

Looks like the ci lint wasn't run before these files made it into main. I'm working on fixing them all now.

wgeorgecook avatar Jun 22 '24 16:06 wgeorgecook

CI is now passing.

wgeorgecook avatar Jun 22 '24 17:06 wgeorgecook

@tmc I resolved conflicts with main for this and ensured that the linter is happy in the /llms directory. Let me know if you want to take this in a different direction but I think this is a good start towards centralizing these error messages.

wgeorgecook avatar Aug 10 '24 14:08 wgeorgecook

Thanks so much, this ia great to see.

tmc avatar Sep 13 '24 00:09 tmc

I wonder if llms is the right place to put the errors, I don't love the new source dependency on what is already a pretty large package implies. I don't have a better suggestion. @eliben do you have an opinion here?

tmc avatar Sep 13 '24 00:09 tmc

@tmc I have the same concern. It feels weird to have a root level errors package since the exported types are directly related to the LLMs. But it doesn't have to be. But it would probably also feel weird to have a root level errors package with just any and all exported errors in it. Very open to suggestions.

wgeorgecook avatar Sep 13 '24 00:09 wgeorgecook

I wonder if llms is the right place to put the errors, I don't love the new source dependency on what is already a pretty large package implies. I don't have a better suggestion. @eliben do you have an opinion here?

I don't see the advantage of this approach - what's the motivation here? How are llm-unique errors going to be represented?

eliben avatar Sep 16 '24 13:09 eliben