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

Fixing Prompt Parser #85

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

Fixing Prompt Parser #85

wants to merge 6 commits into from

Conversation

stevenwalton
Copy link

Prompt parser will get tripped up if "--" is found anywhere in the prompt and isn't surrounded by specific deliminators. Instead let's force arguments to be at the beginning of the prompt and collect no more arguments after. This way users don't have to place flags inside deliminators and we extend what we can pass to the LLMs.

More context is discussed in #52 with an example.

Prompt parser will get tripped up if "--" is found anywhere in the
prompt and isn't surrounded by specific deliminators. Instead let's
force arguments to be at the beginning of the prompt and collect no more
arguments after. This way users don't have to place flags inside
deliminators and we extend what we can pass to the LLMs.

More context is discussed in kharvd#52 with an example.
@stevenwalton
Copy link
Author

Was the problem here the testing? Looks like it failed on building the depends, which I did not modify. I also did not introduce any new packages or imports.

Fwiw, I've been using this version for a month now.

@fn5
Copy link

fn5 commented Sep 6, 2024

@kharvd I ran into an issue with parsing --'s running latest commit d90b1b9 today. Used this fix and it worked perfectly. Could it be merged?

@stevenwalton
Copy link
Author

stevenwalton commented Sep 6, 2024

For now you can pull from my branch since @kharvd has only updated pricing and versioning. But yeah, I'm a bit confused why this isn't being merged either. The build error is on the dependency install (a timeout on one of the packages), so nothing to do with what I touched.

@kharvd can we trigger a rebuild? I didn't check your actions to see what will cause this but I can make a trivial change if that's needed.

@fn5
Copy link

fn5 commented Sep 6, 2024

@stevenwalton Yup, just saw the stale issue :)

@stevenwalton
Copy link
Author

I just synced the branches. If I run into any issues I'll fix or if you find any let me know

@williamjameshandley
Copy link
Contributor

I would also value this being merged. Many thanks for an excellent plugin.

gptcli/__init__.py Outdated Show resolved Hide resolved
@kharvd
Copy link
Owner

kharvd commented Sep 15, 2024

If you are fixing a bug, please add a test case that fails without your fix - and fix the existing tests that are failing.

gptcli/cli.py Outdated Show resolved Hide resolved
- Do not update sub-sub version
- Spelling
- Add test cases and remove old test cases
@stevenwalton
Copy link
Author

Done. Would you like me to update the README?

Fwiw, someone could make this a bit better if there is a need to have an option that takes in several words as --myKey "my option" will not work, but I don't think there is a need for this?

@williamjameshandley
Copy link
Contributor

I would also value this being merged!

Copy link
Contributor

@sghael sghael left a comment

Choose a reason for hiding this comment

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

LGTM (aside from the requested version bump)

gptcli/__init__.py Outdated Show resolved Hide resolved
@stevenwalton
Copy link
Author

For those that keep asking for the merge, in the mean time you can directly pull to my branch. I'll try to keep it in sync but ping me here or there if I have missed this.

Idk if @kharvd has abandoned the project but I'll take PRs into mine.

@kharvd
Copy link
Owner

kharvd commented Nov 17, 2024

sorry for the delay here. I'm inclined to remove inline flags functionality altogether rather than force them to appear at the beginning of the prompt. this has obviously been causing more issues than i'd prefer, and personally i just stopped using these flags altogether.

@stevenwalton
Copy link
Author

Sure, I can understand that. But as of right now, if anyone uses --flag anywhere in their prompt there is a crash. This PR fixes that crash. It is a bug fix.

I get that things take time, but then treat this as a patch. It's not a true "fix" because you're going to later throw it out, but my merging at least people won't be crashing while that's figured out. Truthfully, I don't use those flags either. But I did this because I was annoyed at how often I crashed anytime I'd try to submit requests that involved bash lol

@kharvd
Copy link
Owner

kharvd commented Nov 17, 2024

#100

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