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

potential fix for some/dir/ bug #928 #930

Merged
merged 3 commits into from
Jan 5, 2024

Conversation

ATheorell
Copy link
Collaborator

The problem in bug #928 is that "some/dir/" is used as a placeholder to indicate that a filename should be represented by a relative path. The placeholder is introduced in the preprompt improve. Occasionally, the LLM misunderstands this intention and appends "some/dir/" explicitly to file names. This fix adds a clarification to the LLM the "some/dir/" is a placeholder.

@ErikBjare
Copy link
Collaborator

I think the some/dir/ could perhaps be omitted entirely, and instead mention that it should output a file path, not file name.

Also not sure if this is the right place/time, but I've had great success by writing save codeblocks like this:

```hello.py
print("hello world")
```

And similarly for patch, simply:

```patch hello.py
<<<<<<< ORIGINAL
print("Hello world")
=======
name = input("What is your name? ")
print(f"hello {name}")
>>>>>>> UPDATED
```

I think this might be a superior format overall, but not sure what your experiences with the current one is.

@ATheorell
Copy link
Collaborator Author

Thanks for input @ErikBjare !
I'll have test removing the placeholder entirely. Regarding the syntax you propose, the risk I see is that the ORIGINAL part of the block may occur multiple times in the file, and we will by default replace all, even if we only want to modify one. The current (cumbersome) syntax tries to avoid this by always including a function name (unique by design).

I actually made feature request for a third idea #869
...and really, I have no idea what is the best way to go -> we need a proper benchmarking flow -> which is under construction!

For now, as I said, I will try just removing the placeholder.

@ATheorell
Copy link
Collaborator Author

This should at least partially address the concern in #928, by removing the part of the prompt that misleads the LLM to append some/dir to paths

I have also added an additional test for the case that we are editing a new file (works) and overwriting an existing file (works). This, as I understand should address #952 , which I will close. Correct me if I'm misunderstanding something here @Wheaties466 .

@ATheorell ATheorell merged commit 561b48c into gpt-engineer-org:main Jan 5, 2024
3 checks passed
@miqueet
Copy link

miqueet commented Jan 5, 2024

This should at least partially address the concern in #928, by removing the part of the prompt that misleads the LLM to append some/dir to paths

I have also added an additional test for the case that we are editing a new file (works) and overwriting an existing file (works). This, as I understand should address #952 , which I will close. Correct me if I'm misunderstanding something here @Wheaties466 .

@ATheorell Thank you for the response! Yes, as I understand it. It should address that concern I raised in #952.

If you need or want a test lmk which branch has the fix and I can pull/build/test.

@ATheorell
Copy link
Collaborator Author

Great! I merged to main

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.

None yet

3 participants