AutoGPT icon indicating copy to clipboard operation
AutoGPT copied to clipboard

Migrate to poetry

Open rickythefox opened this issue 1 year ago • 27 comments

Background

Based on https://github.com/Torantulino/Auto-GPT/pull/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

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

rickythefox avatar Apr 13 '23 12:04 rickythefox

@nponeccop please have a look

rickythefox avatar Apr 13 '23 13:04 rickythefox

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?

p-i- avatar Apr 13 '23 15:04 p-i-

@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 https://github.com/Torantulino/Auto-GPT/discussions/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 avatar Apr 13 '23 19:04 rickythefox

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 avatar Apr 14 '23 07:04 rickythefox

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

rickythefox avatar Apr 14 '23 21:04 rickythefox

@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?

p-i- avatar Apr 14 '23 23:04 p-i-

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

rickythefox avatar Apr 15 '23 07:04 rickythefox

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-

rickythefox avatar Apr 15 '23 09:04 rickythefox

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

pymilvus
redis

Please can you add them

Swiftyos avatar Apr 15 '23 15:04 Swiftyos

Conflicting files Dockerfile

richbeales avatar Apr 15 '23 17:04 richbeales

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

rickythefox avatar Apr 15 '23 18:04 rickythefox

@rickythefox thank you very much ! there was some issues on some pipelines plus recent dependencies commented so we kept your work commits and created https://github.com/Significant-Gravitas/Auto-GPT/pull/1736

waynehamadi avatar Apr 15 '23 23:04 waynehamadi

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.

waynehamadi avatar Apr 16 '23 01:04 waynehamadi

@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.

rickythefox avatar Apr 16 '23 10:04 rickythefox

@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.

happysalada avatar Apr 16 '23 20:04 happysalada

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

waynehamadi avatar Apr 16 '23 21:04 waynehamadi

Resolved conflicts

rickythefox avatar Apr 17 '23 09:04 rickythefox

@rickythefox CI is red

nponeccop avatar Apr 17 '23 16:04 nponeccop

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

github-actions[bot] avatar Apr 17 '23 16:04 github-actions[bot]

@nponeccop should be ok now

rickythefox avatar Apr 17 '23 19:04 rickythefox

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

github-actions[bot] avatar Apr 17 '23 19:04 github-actions[bot]

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

github-actions[bot] avatar Apr 18 '23 14:04 github-actions[bot]

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

github-actions[bot] avatar Apr 18 '23 21:04 github-actions[bot]

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

rickythefox avatar Apr 18 '23 21:04 rickythefox

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.

nponeccop avatar Apr 19 '23 15:04 nponeccop

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

rickythefox avatar Apr 19 '23 18:04 rickythefox

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

github-actions[bot] avatar Apr 22 '23 11:04 github-actions[bot]

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

p-i- avatar May 05 '23 00:05 p-i-

Related:

  • #1179
  • #1955

Pwuts avatar Jun 15 '23 15:06 Pwuts

Any news on poetry adoption?

Miuler avatar Jun 21 '23 22:06 Miuler