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

Improved chat parsing with no AI logic #120

Merged
merged 4 commits into from
Jun 18, 2023

Conversation

goncalomoita
Copy link
Contributor

@goncalomoita goncalomoita commented Jun 17, 2023

This fixes compatibility issues in PR #87 that have arisen from recent changes to the repository.
Additionally, an improvement to the parser unit tests has been done.

The goal of the this parser is to make sense of what the LLM has provided without requiring another inference call which adds time and cost overhead. My remarks on this matter were stated in #35.
Further debate has been conducted in PR #94

@patillacode
Copy link
Collaborator

Hi @goncalomoita

thanks a lot for this PR.

I would like to merge this but I need confirmation/feedback from @AntonOsika and anyone else who wants to chime in.

I am thinking that we can merge this and remove it later if #94 gets done. I think #94 fixes this with functions though

@AntonOsika
Copy link
Owner

  • Solves a clear problem ✅
  • Problem is significant ✅
  • Solution is robust ✅

Looks good to me!

@AntonOsika
Copy link
Owner

AntonOsika commented Jun 18, 2023

The only thing is that I always want simpler solutions

  • The simples solution possible ❓

Less code -> easier to change the repository later.

What we could do:

Use a system prompt as a final message that explicitly asks for the format we prefer.

Let me know what you think @goncalomoita, but I'm ready to merge.

@EnzoMartin
Copy link
Contributor

I had a variety of issues with chat parsing, I made similar improvements including supporting directory creation in #140

@patillacode
Copy link
Collaborator

@AntonOsika

Merging now since you agree, we can always simplify if possible later.

Just to be clear I think #94 is the way to go, it is simpler imo and uses the model itself via functions to solve this issue.

Until that is merged (if ever) this provides a solution for an issue that many users have at the moment.

@patillacode patillacode merged commit 8facedd into AntonOsika:main Jun 18, 2023
@patillacode
Copy link
Collaborator

Thank you @goncalomoita

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.

5 participants