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

Integrate mistral.rs LLM #13105

Merged
merged 14 commits into from May 2, 2024
Merged

Integrate mistral.rs LLM #13105

merged 14 commits into from May 2, 2024

Conversation

EricLBuehler
Copy link
Contributor

@EricLBuehler EricLBuehler commented Apr 25, 2024

Description

In this PR, I have added support for the mistral.rs LLM inference platform via a new integration. mistral.rs is a new LLM inference platform with key features such as prefix caching, optimized X-LoRA support, LoRA support via weight merging and grammar support.

New Package?

Did I fill in the tool.llamahub section in the pyproject.toml and provide a detailed README.md for my new integration or package?

  • Yes
  • No

Version Bump?

Did I bump the version in the pyproject.toml file of the package I am updating? (Except for the llama-index-core package)

  • Yes
  • No

Type of Change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added new unit/integration tests
  • Added new notebook (that tests end-to-end)
  • I stared at the code and made sure it makes sense

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran make format; make lint to appease the lint gods

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 25, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@nerdai nerdai left a comment

Choose a reason for hiding this comment

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

Awesome to see rust-based LLM inference libraries!

Took a look, and it seems there is an excessive amount of lint/fmt type of changes in this PR. Not sure they're entirely needed as in that our own static checks should pass with out these.

@EricLBuehler
Copy link
Contributor Author

Thank you! I've removed formatting the whole codebase and just formatted my new changes.

@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 25, 2024
@nerdai
Copy link
Contributor

nerdai commented Apr 25, 2024

Thank you! I've removed formatting the whole codebase and just formatted my new changes.

thanks!

Copy link
Contributor

@nerdai nerdai left a comment

Choose a reason for hiding this comment

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

Left a few comments in my first pass :)

@nerdai
Copy link
Contributor

nerdai commented Apr 25, 2024

@EricLBuehler do you mind adding me to your fork? Looks like we need to do some pants related stuff (i.e. run pants tailor :: in the root of your project)

@EricLBuehler
Copy link
Contributor Author

@nerdai, I addressed your comments and have added you to the repo.

@nerdai
Copy link
Contributor

nerdai commented Apr 26, 2024

@EricLBuehler looks like we're running into error still:

E   SyntaxError: keyword argument repeated: logprobs

@EricLBuehler
Copy link
Contributor Author

@nerdai, sorry for that mistake. It should be fixed now.

@nerdai
Copy link
Contributor

nerdai commented Apr 26, 2024

@nerdai, sorry for that mistake. It should be fixed now.

All good -- thanks for the quick fix!

@EricLBuehler
Copy link
Contributor Author

It seems like the CI tests are failing because this integration depends on the mistralrs library. Would it be best if I update the CI to install and build mistralrs?

@nerdai
Copy link
Contributor

nerdai commented Apr 26, 2024

It seems like the CI tests are failing because this integration depends on the mistralrs library. Would it be best if I update the CI to install and build mistralrs?

yes, please ensure all the required deps are listed in the pyproject.toml. Best to run:

poetry add mistralrs

have you published mistralrs to pypi yet?

@EricLBuehler
Copy link
Contributor Author

@nerdai, I just released mistralrs on pypi. However, it requires a Rust toolchain to build. Can we update the CI to install the Rust toolchain?

@nerdai
Copy link
Contributor

nerdai commented Apr 27, 2024

@nerdai, I just released mistralrs on pypi. However, it requires a Rust toolchain to build. Can we update the CI to install the Rust toolchain?

Ah okay. Can it work with just he standard rust installation?

@EricLBuehler
Copy link
Contributor Author

EricLBuehler commented Apr 27, 2024

@nerdai, yes, it can. It depends on openssl though.

@nerdai
Copy link
Contributor

nerdai commented Apr 28, 2024

@nerdai, yes, it can. It depends on openssl though.

Sorry @EricLBuehler not sure if I'm following. To my knowledge, Rust Toolchain is installed in our github runners by default (source).

Can we not just do a poetry add mistralrs so that it gets added as a dep in pyproject.toml?

@EricLBuehler
Copy link
Contributor Author

@nerdai, thanks for clarifying! I have added it as a dependency now.

@EricLBuehler
Copy link
Contributor Author

EricLBuehler commented May 2, 2024

Hi @nerdai! I have updated this PR to use our latest PyPi release. Additionally, I made sure the tests pass by running the following commands:

make format
make lint
pants tailor --check ::
poetry run make -s test

I think that the CI tests should pass now.

@nerdai
Copy link
Contributor

nerdai commented May 2, 2024

@EricLBuehler we're getting close! Looks like tests are still failing. From the traceback captured in the logs, i see this:

    ) -> list[dict[str, str]]:
E   TypeError: 'type' object is not subscriptable

Maybe we need to do the following:

  • change list to List in all such occurrences in base.py
  • change dict to Dict in all such occurrences in base.py

where List and Dict are both from typing module.

@EricLBuehler
Copy link
Contributor Author

@nerdai, that should be fixed now! Not sure why the tests I ran locally didn't catch that though.

@nerdai
Copy link
Contributor

nerdai commented May 2, 2024

@nerdai, that should be fixed now! Not sure why the tests I ran locally didn't catch that though.

Happens to me too sometimes. I think there's a mismatch between versions perhaps on python or other testing/formatting dependencies. 🤔

@nerdai
Copy link
Contributor

nerdai commented May 2, 2024

@EricLBuehler time to 🛳️!

Thanks for this :)

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 2, 2024
docs/BUILD Outdated Show resolved Hide resolved
@nerdai nerdai merged commit 772a575 into run-llama:main May 2, 2024
8 checks passed
@EricLBuehler
Copy link
Contributor Author

@nerdai thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants