Skip to content
This repository has been archived by the owner on Jun 24, 2024. It is now read-only.

Fix perplexity calculation #251

Merged
merged 7 commits into from
May 20, 2023
Merged

Conversation

tanmaysachan
Copy link
Contributor

There were some errors (one quite big) with #248 for larger files which caused it to panic with a large context window.

I believe this fixes most of those cases. I'm limited in my testing abilities, so if someone else could run perplexity checks for a few models, that would be better.

@philpax
Copy link
Collaborator

philpax commented May 20, 2023

Apologies - I ended up carried away while testing this and rewrote it in the process 😅. While doing so, I discovered a few things:

  1. Perplexity should be treated as a separate CLI command, not run after inference - otherwise, the previous state will interfere with the calculated perplexity. I've made this change.
  2. The calculated perplexity doesn't match up with llama.cpp's, so I ported that one over, and while they're closer, they're still not the same. I suspect this is because of Update to latest upstream LLaMA implementation #210.
  3. The llama.cpp implementation has multiple rounds of perplexity, which are calculated and reported to the user. I've implemented this through a callback.
  4. The implementation is still segfaulty with long enough text (both your implementation and mine). I tested a context length of 512 with about ~10,000 characters, which was sufficient to exceed the context length.

I suspect both 2) and 4) will be resolved by updating the LLaMA implementation. I've made a few interface changes, so I'm inclined to merge this and make an issue to improve its behaviour over time.

Thanks again for the PR, and my sincere apologies about what I did to it - I was trying to get to the bottom of things and realised it was deeper than I expected 😬

@tanmaysachan
Copy link
Contributor Author

@philpax Thanks for the deep dive! I was a little confused about why llama.cpp's perplexity was not matching up with their implementation, because of which I tried implementing it from the source. I'll also have a look at the llama changes to see if I can contribute in fixing this. Thanks for such a great repo!

@philpax philpax merged commit c4b2ca8 into rustformers:main May 20, 2023
@hhamud hhamud mentioned this pull request Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants