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 learning upload #1120

Merged
merged 6 commits into from
Apr 18, 2024
Merged

fix learning upload #1120

merged 6 commits into from
Apr 18, 2024

Conversation

TheoMcCabe
Copy link
Collaborator

@TheoMcCabe TheoMcCabe commented Apr 16, 2024

fixes #1115


Ellipsis 🚀 This PR description was created by Ellipsis for commit 67719c4.

Summary:

This PR fixes issue #1115, fixes the learning upload issue, rearranges the import order in prompt.py, and rearranges the import order in test_learning.py.

Key points:

  • Fixes issue TypeError: Object of type Prompt is not JSON serializable – uploading learning #1115
  • Rearranges the import order in prompt.py
  • Fixes issue with learning upload by using Prompt class in extract_learning function in test_learning.py
  • Rearranges the import order in test_learning.py, moving the import of the Prompt class from gpt_engineer.core.prompt to after the import of DiskMemory from gpt_engineer.core.default.disk_memory

Generated with ❤️ by ellipsis.dev

@TheoMcCabe TheoMcCabe marked this pull request as ready for review April 16, 2024 22:37
Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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 of 50%.
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 [email protected]

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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 of 50%.
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 [email protected]

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 of 50%.
1. /gpt_engineer/core/prompt.py:42:
  • Assessed confidence : 100%
  • Grade: 0%
  • Comment:
    The to_json method should return the JSON string representation of the dictionary returned by the to_dict method. Please add this functionality.
return json.dumps(self.to_dict())
  • Reasoning:
    The to_json method in the Prompt class is not complete. It should return the JSON string representation of the dictionary returned by the to_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 [email protected]

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 of 50%.
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 [email protected]

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 of 50%.
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 [email protected]

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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 of 50%.
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 [email protected]

@TheoMcCabe TheoMcCabe merged commit a40b639 into main Apr 18, 2024
4 checks passed
@TheoMcCabe TheoMcCabe deleted the bug/learning-upload-fails branch April 18, 2024 09:38
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.

TypeError: Object of type Prompt is not JSON serializable – uploading learning
1 participant