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

Migrate to poetry #1102

Closed
wants to merge 4 commits into from
Closed

Conversation

rickythefox
Copy link
Contributor

@rickythefox rickythefox commented Apr 13, 2023

Background

Based on #367

Changes

Migrate to poetry for requirements management.

Documentation

README.md is updated to reflect the change.

Test Plan

$ docker build . -t autogpt-fork
$ docker run -it --rm autogpt-fork   

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

@rickythefox
Copy link
Contributor Author

@nponeccop please have a look

nponeccop
nponeccop previously approved these changes Apr 13, 2023
@p-i-
Copy link
Contributor

p-i- commented Apr 13, 2023

You're fixing all the version numbers in poetry. Isn't half the point of poetry that it can auto-figure-out compatible version numbers?

This was referenced Apr 13, 2023
@richbeales richbeales added the needs discussion To be discussed among maintainers label Apr 13, 2023
@rickythefox
Copy link
Contributor Author

rickythefox commented Apr 13, 2023

@p-i- you're right. We can:

  1. Let poetry figure out the versions freely (PR updated with this). I can submit a PR to update the tests, if we make sure to run them in the CI pipeline we should be pretty safe. See Adding basic CI tests #1117 for potential problem discussion.
  2. ... or pin major versions
  3. ... or just hope any breaking changes will be handled at the time of change

Whats your take on this?

@rickythefox
Copy link
Contributor Author

rickythefox commented Apr 14, 2023

I will sync the fork once we agree on the version handling per above. At the rate this codebase is changing it will look totally different once this PR is approved for merge. =)

@rickythefox
Copy link
Contributor Author

rickythefox commented Apr 14, 2023

@p-i- @richbeales any chance of input on the version resolution question so I can rebase this PR onto the latest master?

@p-i-
Copy link
Contributor

p-i- commented Apr 14, 2023

@rickythefox My vote would be for letting Poetry figure the versions out, rather than pinning versions.

Yes it's possible that some version update could cause an issue, but it feels so much cleaner to keep it floating and fix in the unusual event of a conflict.

How about we stick with (1) for now?

@rickythefox
Copy link
Contributor Author

@p-i- perfect, then my last update handled that II will rebase within a free hours

@rickythefox rickythefox force-pushed the poetry branch 2 times, most recently from 2eeae3f to 07b8a1d Compare April 15, 2023 09:28
@rickythefox
Copy link
Contributor Author

rickythefox commented Apr 15, 2023

Updated and pushed. Note that I pinned playsound = "1.2.2" as 1.3.0 does not seem to build and 1.2.2 was the version used in requirements.txt anyway. If you want I can also include deleting requirements.txt into this PR. @nponeccop @p-i-

@Swiftyos
Copy link
Contributor

@rickythefox
2 deps that are in master are missing from the poetry deps:

pymilvus
redis

Please can you add them

@richbeales
Copy link
Contributor

Conflicting files
Dockerfile

@rickythefox
Copy link
Contributor Author

rickythefox commented Apr 15, 2023

Fixed and added pymilvus and redis. Dev dependencies are not installed when building docker image. @richbeales

richbeales
richbeales previously approved these changes Apr 15, 2023
@waynehamadi
Copy link
Contributor

@rickythefox thank you very much ! there was some issues on some pipelines plus recent dependencies commented so we kept your work commits and created
#1736

@waynehamadi
Copy link
Contributor

waynehamadi commented Apr 16, 2023

Hey @rickythefox we started consolidating your pr into this PR. Then I started thinking about how it could affect regular users and this is my concern:

  • this is a mainstream project, people might not know poetry, they might just have 2 hours of python code behind them (again, I am not sure about that)
  • more things to type for users
    python -m autogpt becomes poetry run python -m autogpt.
  • overhead

So I am not saying no. I am just saying I am not so sure about this being a good move for all users, including non developers / tinkerers / dev amateurs.

@rickythefox
Copy link
Contributor Author

rickythefox commented Apr 16, 2023

@merwanehamadi @BillSchumacher well, the way I see it poetry will be more user-friendly.

  1. Virtual environments are created by default. For someone not familiar with the venv, just using pip to install from requirements.txt would pollute the global python installation and likely lead to conflicts in package versions.
  2. Dependency resolution is nicely handled by poetry.
  3. Typing poetry run takes 10 keystrokes. As long as this is properly documented the problem is minimal. You can also use poetry shell and execute all subsequent commands from there.
  4. I'm not sure you would be able to grasp the codebase with 2 hours of python behind you, poetry or no poetry (prose?). If you have previous experience with java/dotnet/js/clojure you would be familiar with tools like maven/nuget/npm/leiningen which would map nicely to poetry.

@happysalada
Copy link

@merwanehamadi I'm coming to this issue trying to package this for a linux distribution.
Packaging with poetry would make maintainers life much easier, ultimately if you want distribution, having this installable on any distribution with a simple command would be the best IMO.
poetry will on top of that enable build reproducibility, in turn you could check binaries with a sha256 to verify noone tempered with it. I realise this is a minor benefit, but it reproducibility enables several linux distribution to actually package this. For example, it won't be possible to package this into nixos without poetry.

@waynehamadi
Copy link
Contributor

waynehamadi commented Apr 16, 2023

I am fine if we move forward with that, I love poetry, and I was the one pushing for it. As long as we have all users in mind, not just maintainers.

There aren't many open source projects like this that reached such a wide range of users persona

@nponeccop
Copy link
Contributor

@rickythefox CI is red

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 17, 2023
@github-actions
Copy link
Contributor

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

@rickythefox
Copy link
Contributor Author

@nponeccop should be ok now

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 17, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 18, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Apr 18, 2023
@github-actions
Copy link
Contributor

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

@rickythefox
Copy link
Contributor Author

@nponeccop so will this be merged or should I close the PR?

@nponeccop
Copy link
Contributor

I think yes, it will probably be merged, just not now. Plugins are in the works. It's a huge change so we would like to release them first. Probably tonight.

@rickythefox
Copy link
Contributor Author

@nponeccop ok, I'll hold off merging any conflicts until you say go.

@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Apr 22, 2023
@github-actions
Copy link
Contributor

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

@p-i-
Copy link
Contributor

p-i- commented May 5, 2023

This is a mass message from the AutoGPT core team.
Our apologies for the ongoing delay in processing PRs.
This is because we are re-architecting the AutoGPT core!

For more details (and for infor on joining our Discord), please refer to:
https://github.com/Significant-Gravitas/Auto-GPT/wiki/Architecting

@Miuler
Copy link

Miuler commented Jun 21, 2023

Any news on poetry adoption?

@Pwuts
Copy link
Member

Pwuts commented Sep 14, 2023

Getting this up-to-date was too complicated, so I started over in #5219. I've credited you as co-author @rickythefox :)

@Pwuts Pwuts closed this Sep 14, 2023
Pwuts added a commit that referenced this pull request Sep 15, 2023
Inspired by #1102

* Migrate AutoGPT agent to poetry

  Co-authored-by: rickythefox <richard@ginzburg.se>

* Rewrite automatic dependency check (check_requirements.py) for poetry

* Sort dependencies

* Add instructions for poetry to README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Automatically applied to PRs with merge conflicts needs discussion To be discussed among maintainers Obsolete?
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants