Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] - Add support for AI21labs assistants #303

Merged

Conversation

smokestacklightnin
Copy link
Contributor

@smokestacklightnin smokestacklightnin commented Feb 1, 2024

This PR addresses Issue #226 by adding an assistant for AI21 Labs

Please leave opinions and change requests in comments

@smokestacklightnin
Copy link
Contributor Author

@pmeier The first draft of this PR is finished. I'm testing it now.

Opinions and requests to modify are welcome

@smokestacklightnin
Copy link
Contributor Author

smokestacklightnin commented Feb 2, 2024

I'm trying to test this using basic documents and prompts, but I keep getting an error like

[...]/_api.py:28: RuntimeWarning: coroutine 'AI21LabsAssistant._call_api' was never awaited
  async for chunk in self._call_api(  # type: ignore[attr-defined, misc]

I'm trying to debug this, including adding a breakpoint(), but the code has been rather opaque to peer into.
I've tried making a basic AI21Labs API request using a simple standalone script, and that seemed to work.

I'm looking for ideas and input, please

@smokestacklightnin
Copy link
Contributor Author

smokestacklightnin commented Feb 2, 2024

An educated guess is that streaming is not specified. I will test this hypothesis

Edit: Simply adding the stream option to the http request didn't fix the error, although stream isn't part of the API parameters

@smokestacklightnin
Copy link
Contributor Author

smokestacklightnin commented Feb 2, 2024

A later part of the same error gives

File "/home/[...]/ragna/ragna/assistants/_api.py", line 28, in answer
    |     async for chunk in self._call_api(  # type: ignore[attr-defined, misc]
    | TypeError: 'async for' requires an object with __aiter__ method, got coroutine

What's confusing to me is that RagnaException is not getting raised, leading me to believe that the request is (hopefully) coming back successfully. On the other hand, if _call_api isn't being awaited, then something is being held up.

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks @smokestacklightnin for the PR. This already looks pretty good. The issue you are facing has a simple resolution. I've suggested it below along two other minor things.

ragna/assistants/_ai21labs.py Outdated Show resolved Hide resolved
ragna/assistants/_ai21labs.py Outdated Show resolved Hide resolved
ragna/assistants/__init__.py Outdated Show resolved Hide resolved
@smokestacklightnin smokestacklightnin marked this pull request as ready for review February 2, 2024 19:21
Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Almost there just one minor thing below. Apart from that, I'm unable to use the Jurassic2Mid assistant. The server always returns a 500 / Internal Server error for me. Did it work for you? If so, let's wait for them to fix this so I get at least one clean run.

ragna/assistants/_ai21labs.py Outdated Show resolved Hide resolved
@smokestacklightnin
Copy link
Contributor Author

smokestacklightnin commented Feb 2, 2024

Almost there just one minor thing below. Apart from that, I'm unable to use the Jurassic2Mid assistant. The server always returns a 500 / Internal Server error for me. Did it work for you? If so, let's wait for them to fix this so I get at least one clean run.

I get the same 500 http response code for Jurassic2Mid, but Jurassic2Ultra works for me. Is it an option to comment out the Mid model and leave only the Ultra?

@smokestacklightnin
Copy link
Contributor Author

It's also unclear to me why there's a new import error

@pmeier
Copy link
Member

pmeier commented Feb 2, 2024

I get the same 500 http response code for Jurassic2Mid,

Are you saying you are also getting the 500 now or never got a proper response so far? If its the former, I say we wait a few days before we merge this and see if they figure it out on their side. No worries, this PR will make it into the 0.2.0 release

It's also unclear to me why there's a new import error

Well, lancedb==5.0.2 was released an hour ago and our env in CI is likely still cached and does not contain polars. Nothing to worry about on your side.

docs/references/faq.md Outdated Show resolved Hide resolved
ragna/assistants/_ai21labs.py Outdated Show resolved Hide resolved
ragna/assistants/_ai21labs.py Outdated Show resolved Hide resolved
@smokestacklightnin
Copy link
Contributor Author

Are you saying you are also getting the 500 now or never got a proper response so far? If its the former, I say we wait a few days before we merge this and see if they figure it out on their side. No worries, this PR will make it into the 0.2.0 release

The former

@pmeier
Copy link
Member

pmeier commented Feb 2, 2024

In that case let's revisit this next week and see if they have fixed their server.

ragna/assistants/_ai21labs.py Show resolved Hide resolved
ragna/assistants/_ai21labs.py Show resolved Hide resolved
@pmeier
Copy link
Member

pmeier commented Feb 7, 2024

I've tried again today and I'm still getting the 500 error. Since the release is still at least a week out I'm ok with waiting. However, also fine to just remove JurrassicMid and only add it again later.

@smokestacklightnin
Copy link
Contributor Author

I've tried again today and I'm still getting the 500 error. Since the release is still at least a week out I'm ok with waiting. However, also fine to just remove JurrassicMid and only add it again later.

I'm mostly fine with either option.

I tested JurassicUltra with a text file that reads

Roses are red
Violets are blue

and then asked the system the question

How many legs does a spider have?

to which the system responded

A spider has eight legs.

While this is true, it doesn't feel right that the information provided didn't come from the included documents, even though the system is only supposed to answer the questions from what's included. I double checked the documentation (this and this), and everything seems to be set up correctly in the code from this PR. What are your thoughts?

@pmeier
Copy link
Member

pmeier commented Feb 7, 2024

I'm mostly fine with either option.

In that case let's wait a little longer. If we get closer to release and still have no green light, we can always pull the trigger then.

While this is true, it doesn't feel right that the information provided didn't come from the included documents, even though the system is only supposed to answer the questions from what's included.

Unfortunately, this is simply how LLMs work. There is no way to force them into any behavior. The system prompt just gives an instruction to it. Whether or not this is respected is a completely different story. IMO this is a quality metric and it seems that the Jurassic model is not on par with the other LLMs that we have built in.

Unless it actually can't answer a question even if given the right context, I still think it makes sense to include it.

@pmeier pmeier linked an issue Feb 9, 2024 that may be closed by this pull request
@smokestacklightnin smokestacklightnin force-pushed the ai21labs-assistant/basic-functionality branch from 80c1169 to 0a813b6 Compare February 12, 2024 21:56
@smokestacklightnin
Copy link
Contributor Author

The 500 internal service error is still present from AI21labs, but I've rebased this branch to get rid of the conflict in the FAQ documentation file

@pmeier
Copy link
Member

pmeier commented Feb 12, 2024

Up to you how you want to handle this: keep waiting for them to fix their server or remove the "mid" model from the PR and we can merge as is.

@smokestacklightnin
Copy link
Contributor Author

Up to you how you want to handle this: keep waiting for them to fix their server or remove the "mid" model from the PR and we can merge as is.

I'd love to have another PR merge on the books. If you don't see any reason not to comment out the Mid model, I'm happy to go ahead and do so to make this PR ready to merge

@pmeier
Copy link
Member

pmeier commented Feb 12, 2024

Please leave a comment on the "mid" model on why this is commented out linking to this thread.

@smokestacklightnin
Copy link
Contributor Author

Please leave a comment on the "mid" model on why this is commented out linking to this thread.

I added a comment. I'll add a link to this thread

@smokestacklightnin
Copy link
Contributor Author

Please leave a comment on the "mid" model on why this is commented out linking to this thread.

Done

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

@pmeier pmeier merged commit 9304349 into Quansight:main Feb 13, 2024
10 checks passed
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.

[ENH] - Add support for AI21labs assistants
2 participants