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

Refactoring needles tester #28

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

LazaroHurtado
Copy link
Contributor

There are some duplicated methods/logic in LLMMultiNeedleHaystackTester, this PR is to clean this up. Furthermore, some logging lines get outputted multiple times due to coroutines from asyncio. It is best to remove these lines since they only print what the user has already specified, so no extra information is provided. In the future I plan on adding some debugging support depending on the debug mode main.py was called with.

@LazaroHurtado
Copy link
Contributor Author

LazaroHurtado commented Mar 11, 2024

@kedarchandrayan, @pavelkraleu #19 introduced some duplicate methods in main.py which have already diverged, for example, one get_evaluator method uses args.evaluator_model_name while another doesn't. Can I please get a review on this PR to solve this issue

@kedarchandrayan
Copy link
Collaborator

Hello @LazaroHurtado, thanks for catching the duplicate code. We have made changes in another fork regarding the same already.

@LazaroHurtado
Copy link
Contributor Author

Hello @LazaroHurtado, thanks for catching the duplicate code. We have made changes in another fork regarding the same already.

@kedarchandrayan glad to hear! Shouldn't the source repo contain these fixes though?

@kedarchandrayan
Copy link
Collaborator

Yup, the fork will be merged today!

@kedarchandrayan
Copy link
Collaborator

Hi @LazaroHurtado, a big change was made today (#33) that has made this pull request outdated. I apologize for the inconvenience. If it is easier, please feel free to open a new pull request with the changes using the latest code.

@LazaroHurtado
Copy link
Contributor Author

Hi @LazaroHurtado, a big change was made today (#33) that has made this pull request outdated. I apologize for the inconvenience. If it is easier, please feel free to open a new pull request with the changes using the latest code.

I also updated this PR 😄

@pavelkraleu pavelkraleu added the enhancement New feature or request label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants