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

Fix: Crashes after entering additional questions #769

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Fix: Crashes after entering additional questions #769

merged 3 commits into from
Oct 6, 2023

Conversation

pralad-p
Copy link
Contributor

@pralad-p pralad-p commented Oct 5, 2023

Fixes #754.

Cause:

From a recent Langchain update, it seems the AIMessage object has a much stricter schema which only allows is_chunk attributes to have a False value. However, in gpt_engineer, the deserialization of clarify logs implicitly sets is_chunk to True.

Proposed fixes:

  1. Either go to Langchain (or Pydantic which they use internally for model validation of their types) and loosen their requirements for an AIMessage object model. They may or may not agree with my proposal.
  2. Make it explicit that when message objects are deserialized from the JSON string, that their is_chunk attribute can never be True.

Containment/Fix:

I go with Fix 2.
This commit deserializes and stores an intermediate list, from which we explicitly set all is_chunk values to False. Since the Langchain messages_from_dict(..) method anyways expect a Message object with an is_chunk property of False, we don't have the effect of hotfixing this property, but rather add an additional check to make sure Messages have valid properties (like this is_chunk property for e.g.).

@ATheorell
Copy link
Collaborator

Thanks for picking this up. Just answer my minor question and I will merge.

@pralad-p
Copy link
Contributor Author

pralad-p commented Oct 6, 2023

Thanks for picking this up. Just answer my minor question and I will merge.

Thanks for reviewing this. But pardon me, I missed your question?

@ATheorell
Copy link
Collaborator

Is the call on line 349 really necessary? Can't you just directly put modified_data in the return and omit the json.loads statement on the following line?
It looks like we are serializing and deserializing on two consecutive lines.

@pralad-p
Copy link
Contributor Author

pralad-p commented Oct 6, 2023

Is the call on line 349 really necessary? Can't you just directly put modified_data in the return and omit the json.loads statement on the following line? It looks like we are serializing and deserializing on two consecutive lines.

You're absolutely right, I was fixated on splitting out the steps in case in the future Langchain strictens their schema even more that I didn't even realize I do an extra step for no reason. Thanks. I'll implement a fix now with a sanity check.

@ATheorell ATheorell merged commit de45398 into gpt-engineer-org:main Oct 6, 2023
4 checks passed
@ATheorell
Copy link
Collaborator

Thanks, great that you dug out the error.

@pralad-p
Copy link
Contributor Author

pralad-p commented Oct 6, 2023

Done ✅ I would also recommend to put this in a minor release for its PyPi package soon (I'm not sure what your release cycles are like), because this breaks the application whenever the user is asked for clarification. Even if the user skips.

@pralad-p
Copy link
Contributor Author

pralad-p commented Oct 6, 2023

Thanks, great that you dug out the error.

No problem, glad I could help 😄

@pralad-p pralad-p deleted the additional-question-fix branch October 6, 2023 14:30
@bhanuunrivalled
Copy link

gpt-engineer 0.0.9

i have this problem tooo

in which version this is solved?

@ATheorell
Copy link
Collaborator

ATheorell commented Oct 11, 2023

Thanks for reporting!
So, it is solved on commit d37feec
We currently do not update the version in each commit, so many commits are called 0.0.9. If you refer to the version on pypi, this bug still persists.

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.

the code fails after giving additional information at the questions.
3 participants