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

Fixed revising constraints and best practices #6777

Merged

Conversation

ThunderDrag
Copy link
Contributor

@ThunderDrag ThunderDrag commented Feb 1, 2024

Background

When using AutoGPT I noticed that it starts to skip constraints and best practices when you use "-" to remove them, it seems like that happens because the constraints are removed in the for loop immediately thus shortening the list and skipping over other constraints.

Changes 🏗️

I have made an list for constraints and best practices each, the ones designated for removal are added to that list and removed after the for loop has finished

PR Quality Scorecard ✨

  • Have you used the PR description template?   +2 pts
  • Is your pull request atomic, focusing on a single change?   +5 pts
  • Have you linked the GitHub issue(s) that this PR addresses?   +5 pts
  • Have you documented your changes clearly and comprehensively?   +5 pts
  • Have you changed or added a feature?   -4 pts
    • Have you added/updated corresponding documentation?   +4 pts
    • Have you added/updated corresponding integration tests?   +5 pts
  • Have you changed the behavior of AutoGPT?   -5 pts
    • Have you also run agbenchmark to verify that these changes do not regress performance?   +10 pts

@ThunderDrag ThunderDrag requested a review from Pwuts as a code owner February 1, 2024 09:52
@github-actions github-actions bot added the size/m label Feb 1, 2024
Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for auto-gpt-docs ready!

Name Link
🔨 Latest commit fe72973
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/65d474c23261c50008ef1834
😎 Deploy Preview https://deploy-preview-6777--auto-gpt-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Pwuts
Copy link
Member

Pwuts commented Feb 1, 2024

You're right about the bug, a for loop doesn't like in-place removal of items. Nice catch!

Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

The fix could be a bit simpler though, by turning the for loop into a while loop, like this:

        # Revise constraints
-       for i, constraint in enumerate(directives.constraints):
+       i = 0
+       while i < len(directives.constraints)
+           constraint = directives.constraints[i]
            print_attribute(f"Constraint {i+1}:", f'"{constraint}"')
            new_constraint = (
                await clean_input(
                    app_config,
                    f"Enter new constraint {i+1}"
                    " (press enter to keep current, or '-' to remove):",
                )
                or constraint
            )
            if new_constraint == "-":
                directives.constraints.remove(constraint)
+               continue
            elif new_constraint:
                directives.constraints[i] = new_constraint
+           i += 1

@Pwuts Pwuts added bug Something isn't working Classic AutoGPT Agent labels Feb 1, 2024
…for iterating over constraints, resources, and best practices
@ThunderDrag
Copy link
Contributor Author

Made the requested changes

@ThunderDrag ThunderDrag requested a review from Pwuts February 1, 2024 11:36
@Pwuts
Copy link
Member

Pwuts commented Feb 2, 2024

Please run poetry run black . && poetry run flake8 . (in autogpts/autogpt folder): https://github.com/Significant-Gravitas/AutoGPT/actions/runs/7759553508/job/21164192021?pr=6777

@github-actions github-actions bot added size/l and removed size/m labels Feb 2, 2024
@ThunderDrag
Copy link
Contributor Author

I have ran the commands

Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

I'm not sure what happened, but it looks like the linter ran with the wrong settings (probably because of wrong CWD). Diff now contains a lot of code formatting changes that are outside the scope of the PR.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Feb 14, 2024
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

@ntindle
Copy link
Member

ntindle commented Feb 15, 2024

You can probably revert this commit and try again 2bd8a50

@ThunderDrag ThunderDrag force-pushed the fix-changing-constraints branch from 2bd8a50 to 784e2bb Compare February 18, 2024 15:05
@github-actions github-actions bot added size/xs and removed size/l labels Feb 18, 2024
@ThunderDrag
Copy link
Contributor Author

Rebased it

@ThunderDrag ThunderDrag reopened this Feb 18, 2024
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Feb 18, 2024
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the size/xs label Feb 18, 2024
@Pwuts Pwuts self-assigned this Feb 20, 2024
@Pwuts Pwuts added this to the AutoGPT v0.6.0 milestone Feb 20, 2024
@Pwuts Pwuts merged commit 49a6d68 into Significant-Gravitas:master Feb 20, 2024
10 of 13 checks passed
@Pwuts
Copy link
Member

Pwuts commented Feb 20, 2024

Thank you very much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants