-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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 learning upload #1120
fix learning upload #1120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on 6e355b5
- Looked at
56
lines of code in2
files - Took 1 minute and 51 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. gpt_engineer/applications/cli/learning.py:269
:
- Assessed confidence :
0%
- Comment:
The change to store the prompt as a JSON string instead of a Prompt object seems to be correct. The to_json() method on the Prompt object correctly returns a JSON string representation of the object. - Reasoning:
The change in the PR is to store the prompt as a JSON string instead of a Prompt object. This is done by calling the to_json() method on the Prompt object, which was added in this PR. The to_json() method calls the to_dict() method and then converts the dictionary to a JSON string. The to_dict() method returns a dictionary with the attributes of the Prompt object. This change seems to be correct and I don't see any issues with it.
Workflow ID: wflow_Q1t1VUSCWrl0jt0E
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
⌛ You have 4 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Reviewed the entire pull request up to 6e355b5
- Looked at
57
lines of code in2
files - Took 1 minute and 19 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. gpt_engineer/applications/cli/learning.py:101
:
- Assessed confidence :
50%
- Comment:
The change from type Prompt to str for the 'prompt' attribute in the Learning class is a significant change. It changes the way the prompt is stored and used in the Learning class. This change will affect all parts of the codebase where the Learning class is used and the 'prompt' attribute is accessed. Please ensure that these changes are reflected everywhere in the codebase where necessary. - Reasoning:
The change in the Learning class attribute 'prompt' from type Prompt to str is a significant change. It changes the way the prompt is stored and used in the Learning class. The PR author has added a to_json method in the Prompt class to convert the prompt to a JSON string before storing it in the Learning class. This change seems to be consistent and correctly implemented across the codebase. However, it's important to note that this change will affect all parts of the codebase where the Learning class is used and the 'prompt' attribute is accessed. The PR author should ensure that these changes are reflected everywhere in the codebase where necessary.
Workflow ID: wflow_TrhXx8dwex4DxCG2
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
⌛ You have 4 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on 0591852
- Looked at
19
lines of code in1
files - Took 2 minutes and 10 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. /gpt_engineer/core/prompt.py:42
:
- Assessed confidence :
100%
- Grade:
0%
- Comment:
Theto_json
method should return the JSON string representation of the dictionary returned by theto_dict
method. Please add this functionality.
return json.dumps(self.to_dict())
- Reasoning:
Theto_json
method in thePrompt
class is not complete. It should return the JSON string representation of the dictionary returned by theto_dict
method.
Workflow ID: wflow_0cxWStH1RbNBgSZ0
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
⌛ You have 4 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on fac36c6
- Looked at
13
lines of code in1
files - Took 2 minutes and 28 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. gpt_engineer/core/prompt.py:1
:
- Assessed confidence :
100%
- Grade:
0%
- Comment:
The PR doesn't seem to have any changes related to the issue. The issue is about the 'Prompt' class not being serializable, but the PR only rearranges the import statements. There's no change in the 'Prompt' class or its methods that could potentially fix the issue. - Reasoning:
The PR doesn't seem to have any changes related to the issue. The issue is about the 'Prompt' class not being serializable, but the PR only rearranges the import statements. There's no change in the 'Prompt' class or its methods that could potentially fix the issue.
Workflow ID: wflow_1hfHTgfaOlKjnPku
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
⌛ You have 4 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on 473ea8d
- Looked at
20
lines of code in1
files - Took 8 minutes and 48 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. /tests/applications/cli/test_learning.py:90
:
- Assessed confidence :
100%
- Grade:
0%
- Comment:
The 'Prompt' class does not seem to have a method to convert it to a JSON serializable format. Consider adding a method to the 'Prompt' class to convert it to a JSON serializable format. - Reasoning:
The 'Prompt' class does not seem to have a method to convert it to a JSON serializable format. This could potentially cause issues when trying to serialize an instance of the 'Prompt' class. The PR author should consider adding a method to the 'Prompt' class to convert it to a JSON serializable format.
Workflow ID: wflow_OKtDShl5yKB1sJIt
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
⌛ You have 4 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
- Performed an incremental review on 67719c4
- Looked at
15
lines of code in1
files - Took 1 minute and 38 seconds to review
More info
- Skipped
0
files when reviewing. - Skipped posting
1
additional comments because they didn't meet confidence threshold of50%
.
1. tests/applications/cli/test_learning.py:6
:
- Assessed confidence :
33%
- Comment:
The import order change doesn't seem to affect the functionality of the code. However, it's a good practice to keep the imports organized and grouped by their origin. Consider reverting this change if it's not necessary for the fix. - Reasoning:
The PR changes the import order of the 'Prompt' class from 'gpt_engineer.core.prompt'. This change doesn't seem to affect the functionality of the code, as the 'Prompt' class is not used in this file. However, it's a good practice to keep the imports organized and grouped by their origin (standard library, third-party libraries, local application/library specific imports).
Workflow ID: wflow_SONhT0Ja14IAoNFq
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
⌛ You have 4 days remaining in your free trial. Upgrade at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at help@ellipsis.dev
fixes #1115
Summary:
This PR fixes issue #1115, fixes the learning upload issue, rearranges the import order in
prompt.py
, and rearranges the import order intest_learning.py
.Key points:
prompt.py
Prompt
class inextract_learning
function intest_learning.py
test_learning.py
, moving the import of thePrompt
class fromgpt_engineer.core.prompt
to after the import ofDiskMemory
fromgpt_engineer.core.default.disk_memory
Generated with ❤️ by ellipsis.dev