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

Use Poetry in awssh #26

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
Draft

Use Poetry in awssh #26

wants to merge 6 commits into from

Conversation

jasonodoom
Copy link
Contributor

🗣 Description

Considering the need for awssh, I found the installation process tedious and figured since I was to create a PR to add to the README I should contribute more to the repo. I initially was to create a flake.nix but decided to also include poetry.

💭 Motivation and context

There are many benefits to using poetry as opposed to setup.pyand virtualenv. Besides having less files (as you can see from the ones I've removed). Did you know you can (in the example of awssh) execute the program via poetry run awssh without having to set-up a virtualenv? Poetry does that for you among other things! Let's discuss.

🧪 Testing

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Create a release.

@jasonodoom jasonodoom added improvement This issue or pull request will add or improve functionality, maintainability, or ease of use dependencies Pull requests that update a dependency file labels Jul 20, 2023
@jasonodoom jasonodoom marked this pull request as draft July 20, 2023 00:05
Copy link
Member

@mcdonnnj mcdonnnj 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 still not sold on using one of these project managers, but if we were going that route I would prefer we use something more "official" like Hatch. Poetry is not PEP-621 compliant and due to design decision that introduced incompatible elements to pyproject.toml it will take time to become compliant. It also has a poor history of semver compliance (bugs/breaking changes introduced on minor or even patch releases).

@jasonodoom
Copy link
Contributor Author

I'm still not sold on using one of these project managers, but if we were going that route I would prefer we use something more "official" like Hatch. Poetry is not PEP-621 compliant and due to design decision that introduced incompatible elements to pyproject.toml it will take time to become compliant. It also has a poor history of semver compliance (bugs/breaking changes introduced on minor or even patch releases).

Those two aren't necessarily the same thing though. Sure, it will take some time but they're working on it. Until then, it's not the end of the world. I think this is something we should seriously consider adopting. Our current process is old. This will be more beneficial and save lots of time especially with maintenance. Also, if you do more research into Poetry you will find it shares some features with Hatch as well. Lack of PEP-621 compliance isn't a very good reason to not use it.

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

How does poetry compare with pipenv? Do we have an indication as to what will likely become the standard tool?

I see that some well-established Python packages still use setuptools, others use pipenv, and still others use poetry. We should probably have a team discussion about this.

Comment on lines +15 to +23
[tool.poetry.group.dev.dependencies]
boto3-stubs = "^1.28.5"
coverage = "^7.2.7"
coveralls = "!= 1.11.0"
pre-commit = "^3.3.3"
pytest-cov = "^4.1.0"
pytest = "^7.4.0"
types-docopt = "^0.6.11.3"
types-requests = "^2.31.0.1"
Copy link
Member

@jsf9k jsf9k Jul 20, 2023

Choose a reason for hiding this comment

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

We previously had (via the requirements*.txt files and setup.py) installation, testing, and development modes. (Dev packages are a superset of the testing packages which are themselves a superset of the installation packages.) Is that not possible in Poetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does poetry compare with pipenv? Do we have an indication as to what will likely become the standard tool?
Poetry will create and manage a virtual env for you. It's faster than pipenv and is better at resolving dependencies. https://python-poetry.org/docs/managing-environments/

We previously had (via the requirements*.txt files and setup.py) installation, testing, and development installations. (Dev packages are a superset of the testing packages which are themselves a superset of the installation packages.) Is that not possible in Poetry?

Yes this is possible. We can group them just like the above. Something like: poetry add pytest@latest --group dev And then it will create a section in the toml for dev dependencies or testing if you want based on specification.

@jsf9k
Copy link
Member

jsf9k commented Jul 20, 2023

I'm still not sold on using one of these project managers, but if we were going that route I would prefer we use something more "official" like Hatch. Poetry is not PEP-621 compliant and due to design decision that introduced incompatible elements to pyproject.toml it will take time to become compliant. It also has a poor history of semver compliance (bugs/breaking changes introduced on minor or even patch releases).

I guess what's not clear to me is if there really is anything "official" right now. I see both pipenv and poetry widely adopted. I wasn't aware of hatch.

Personally, I don't have any issue with using multiple tools like we do now with pyenv + virtualenv + setuptools. So the fact that one tool can magically create the venv (for example) isn't a big selling point for me. But keeping current with our Python packaging is a selling point for me. Other useful features, like a lockfile, will also earn points with me.

@jasonodoom
Copy link
Contributor Author

I'm still not sold on using one of these project managers, but if we were going that route I would prefer we use something more "official" like Hatch. Poetry is not PEP-621 compliant and due to design decision that introduced incompatible elements to pyproject.toml it will take time to become compliant. It also has a poor history of semver compliance (bugs/breaking changes introduced on minor or even patch releases).

I guess what's not clear to me is if there really is anything "official" right now. I see both pipenv and poetry widely adopted. I wasn't aware of hatch.

Personally, I don't have any issue with using multiple tools like we do now with pyenv + virtualenv + setuptools. So the fact that one tool can magically create the venv (for example) isn't a big selling point for me. But keeping current with our Python packaging is a selling point for me. Other useful features, like a lockfile, will also earn points with me.

Sounds good. Yeah, there is no official tooling. These are just tools that are gaining popularity because they're more modern.

So the fact that one tool can magically create the venv (for example) isn't a big selling point for me. But keeping current with our Python packaging is a selling point for me. Other useful features, like a lockfile, will also earn points with me.

I understand, personally, I find creating venv annoying but Poetry I can say does keep current with out current packaging and I'd argue will make the process more simpler and will take less than an hour to learn.

@mcdonnnj
Copy link
Member

I'm still not sold on using one of these project managers, but if we were going that route I would prefer we use something more "official" like Hatch. Poetry is not PEP-621 compliant and due to design decision that introduced incompatible elements to pyproject.toml it will take time to become compliant. It also has a poor history of semver compliance (bugs/breaking changes introduced on minor or even patch releases).

Those two aren't necessarily the same thing though. Sure, it will take some time but they're working on it. Until then, it's not the end of the world. I think this is something we should seriously consider adopting. Our current process is old. This will be more beneficial and save lots of time especially with maintenance. Also, if you do more research into Poetry you will find it shares some features with Hatch as well. Lack of PEP-621 compliance isn't a very good reason to not use it.

I argue instead that poetry being specifically PEP-621 non-compliant is important and is a reason some projects have moved away from poetry. I mention hatch as "official" because it is owned by the Python Packaging Authority (PyPA). I am of the opinion that pipenv solves a different problem in that it provides a repeatable deployment for a Python application environment. That is different from providing dependency management for a Python package. I'm all for migrating to pyproject.toml (I just haven't had time to do a PR) but I'm much less sold on a project manager, let alone something as heavy as poetry.

@jsf9k
Copy link
Member

jsf9k commented Jul 20, 2023

I'm still not sold on using one of these project managers, but if we were going that route I would prefer we use something more "official" like Hatch. Poetry is not PEP-621 compliant and due to design decision that introduced incompatible elements to pyproject.toml it will take time to become compliant. It also has a poor history of semver compliance (bugs/breaking changes introduced on minor or even patch releases).

Those two aren't necessarily the same thing though. Sure, it will take some time but they're working on it. Until then, it's not the end of the world. I think this is something we should seriously consider adopting. Our current process is old. This will be more beneficial and save lots of time especially with maintenance. Also, if you do more research into Poetry you will find it shares some features with Hatch as well. Lack of PEP-621 compliance isn't a very good reason to not use it.

I argue instead that poetry being specifically PEP-621 non-compliant is important and is a reason some projects have moved away from poetry. I mention hatch as "official" because it is owned by the Python Packaging Authority (PyPA). I am of the opinion that pipenv solves a different problem in that it provides a repeatable deployment for a Python application environment. That is different from providing dependency management for a Python package. I'm all for migrating to pyproject.toml (I just haven't had time to do a PR) but I'm much less sold on a project manager, let alone something as heavy as poetry.

I'm 100% onboard with moving to pyproject.toml.

Based on the reading I've been doing this morning, I also lean toward hatch over poetry - primarily because of the adherence to PEPs and also because it is blessed by PyPA. That said, I don't see a personal need for a project manager; at the same time, using a project manager could make it much simpler for third party devs to start contributing. If they could set up their environment with a command or two using a well-supported tool then that might make the setup process less daunting for them. I think that's an advantage worth considering.

@dav3r
Copy link
Member

dav3r commented Jul 21, 2023

Sorry @jasonodoom, but I am in agreement with @mcdonnnj and @jsf9k here. It sounds like hatch is a safer bet for us. PEP compliance isn't all-important, but between two similar tools, I'd rather bet on the compliant one that is endorsed by PyPA. There's nothing stopping us from changing course later if we determine that poetry is more advantageous for us to use. If nothing else, this repo can be a good experiment for us to use hatch and see how we like it.

That being said, I don't love this nugget in their FAQ:

The only caveat is that currently there is no support for re-creating an environment given a set of dependencies in a reproducible manner. Although a standard lock file format may be far off since PEP 665 was rejected, resolving capabilities are coming to pip. When that is stabilized, Hatch will add locking functionality and dedicated documentation for managing applications.

However, since we currently don't already have that capability across our skeletons, I don't think this is a deal-breaker.

@jasonodoom
Copy link
Contributor Author

Sorry @jasonodoom, but I am in agreement with @mcdonnnj and @jsf9k here. It sounds like hatch is a safer bet for us. PEP compliance isn't all-important, but between two similar tools, I'd rather bet on the compliant one that is endorsed by PyPA. There's nothing stopping us from changing course later if we determine that poetry is more advantageous for us to use. If nothing else, this repo can be a good experiment for us to use hatch and see how we like it.

That being said, I don't love this nugget in their FAQ:

The only caveat is that currently there is no support for re-creating an environment given a set of dependencies in a reproducible manner. Although a standard lock file format may be far off since PEP 665 was rejected, resolving capabilities are coming to pip. When that is stabilized, Hatch will add locking functionality and dedicated documentation for managing applications.

However, since we currently don't already have that capability across our skeletons, I don't think this is a deal-breaker.

I'll take a look at Hatch and see what I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants