Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Switch packaging from setuptools to poetry #6

Merged
merged 5 commits into from
Feb 11, 2022

Conversation

greschd
Copy link
Member

@greschd greschd commented Feb 9, 2022

Switch the build backend from setuptools to flit, and move the information in setup.py and the requirements_*.txt files to pyproject.toml.

pyproject.toml Outdated Show resolved Hide resolved
@akaszynski
Copy link
Contributor

If you're feeling up to it, the dev-guide will need an updated section regarding packaging. Let me know if you want to take a stab at that.

@greschd greschd marked this pull request as ready for review February 9, 2022 09:50
@greschd
Copy link
Member Author

greschd commented Feb 9, 2022

If you're feeling up to it, the dev-guide will need an updated section regarding packaging. Let me know if you want to take a stab at that.

I can change the things updated here, if there are other necessary changes (maybe to the token part?) I'll leave that to someone else.

@da1910
Copy link

da1910 commented Feb 9, 2022

A couple of general comments:

  1. There is a preference within the packaging project for a /src based layout, where the code is within a src directory. (See Stance (or discussion) on src/ directory pypa/packaging.python.org#320 for example). This may require a single change to the toml file, but I can't remember OTTOMH which runes flit requires.
  2. At the moment Dependabot is not able to parse dependencies listed in flit format, the only build backends that are supported are setuptools (via requirements.txt) and poetry.
  3. I would advocate for providing a sample tox or nox configuration and recommending its use for testing, it makes managing virtual environments for test runs much simpler. There are drawbacks when nonstandard configurations are required however, so we ought to discuss whether it's likely that we will want to support these (private package repos for example).

@greschd
Copy link
Member Author

greschd commented Feb 9, 2022

A couple of general comments:

1. There is a preference within the packaging project for a `/src` based layout, where the code is within a `src` directory. (See [Stance (or discussion) on src/ directory pypa/packaging.python.org#320](https://github.com/pypa/packaging.python.org/issues/320) for example). This may require a single change to the toml file, but I can't remember OTTOMH which runes flit requires.

I don't have a strong opinion either way on this, happy to switch to src/ if there's consensus on that. Went looking for pro-src/ arguments, there's a list here: https://blog.ionelmc.ro/2014/05/25/python-packaging/#the-structure

2. At the moment Dependabot is not able to parse dependencies listed in flit format, the only build backends that are supported are setuptools (via requirements.txt) and poetry.

Hmm, do you know if that's true for flit's "new-style metadata", or only the older flit.ini format? AFAICT, the new dependencies metadata is exactly as defined in PEP 621.

3. I would advocate for providing a sample tox or nox configuration and recommending its use for testing, it makes managing virtual environments for test runs much simpler. There are drawbacks when nonstandard configurations are required however, so we ought to discuss whether it's likely that we will want to support these (private package repos for example).

I have no experience with tox, is this something you could contribute to?

@akaszynski
Copy link
Contributor

There is a preference within the packaging project for a /src based layout, where the code is within a src directory. (See Stance (or discussion) on src/ directory pypa/packaging.python.org#320 for example). This may require a single change to the toml file, but I can't remember OTTOMH which runes flit requires.

That's a spicy discussion, and I found it better to read the blog post here: https://hynek.me/articles/testing-packaging/

The biggest advantage using src according to Testing & Packaging IMO:

Your tests do not run against the package as it will be installed by its users. They run against whatever the situation in your project directory is.

This occurs quite often (by accident) unless you cd out of the root directory or run your github action in a different directory. I'm leaning towards using the src directory.

At the moment Dependabot is not able to parse dependencies listed in flit format, the only build backends that are supported are setuptools (via requirements.txt) and poetry.

Perhaps that's a reason to recommend poetry over flit, especially as dependabot is a great feature to be able to use, but as @greschd pointed out, we should ideally be able to use as I like how simple flit is.

I would advocate for providing a sample tox or nox configuration

This was echoed by @ninad-kamat and is being used for PyPRIME.


Follow-up PR and discussion points:

  1. Let's talk during the weekly discussion and get a consensus regarding src
  2. @greschd and @da1910, we need to see if we can get dependabot working for flit because I feel that's quite a feature to miss out on.
  3. Bring up the use of tox in the weekly meeting and then have a follow-up PR.

@greschd
Copy link
Member Author

greschd commented Feb 9, 2022

@akaszynski for the dependabot question, I think we just need to try it out.. do you already have a dummy project to test dependabot somewhere, or should I create one?

@da1910
Copy link

da1910 commented Feb 9, 2022

Dependabot on github is not exactly the same code as the public https://github.com/dependabot/dependabot-core repo, it's a little behind development, but there is no support for build or flit backends. The only one it will parse and report updates on is poetry.

We could look at hosting it ourselves, and I looked into this internally, but it's very much not a plug-and-play solution.

@ninad-kamat
Copy link

@akaszynski I did experiment with tox and it is pretty good. I am actually planning to move our CI/CD internally to move to tox. This allows me to use the same command on CI/CD as I would during development and also control the environment where the specific scenario is run. It allows a really easy framework to test different versions of python and have doc, style check, coverage or any other items that we would want to cover under a simple tox.ini configuration file. I think having a tox file is definitely a good idea.

@greschd
Copy link
Member Author

greschd commented Feb 9, 2022

Ok, shall we postpone merging this until after tomorrow's discussion? For tox, we should probably open a separate issue to discuss details.

@akaszynski
Copy link
Contributor

Ok, shall we postpone merging this until after tomorrow's discussion? For tox, we should probably open a separate issue to discuss details.

Separate issue/PR for tox would be valuable. I've only used tox once or twice and would find it useful to have here in the template.

I like the python approach of

There should be one-- and preferably only one --obvious way to do it.

I hope we can at least present our opinion of that approach, especially as it can be hard to settle on a standard given the number of competing and changing approaches out there.

@ninad-kamat
Copy link

@akaszynski @greschd @da1910 Created #7 to track the tox discussion.

@akaszynski
Copy link
Contributor

Let's wait to merge this until we check out poetry.

@greschd
Copy link
Member Author

greschd commented Feb 10, 2022

I have played around with dependabot a bit here: https://github.com/pyansys/test-dependabot-repo/pulls

My takeaways are:

  • as mentioned by @da1910, flit is not supported
  • for poetry, two version updating schemes are supported (see the docs):
    • auto (the default) increases the version in both pyproject.toml and poetry.lock to the latest version
    • lockfile-only updates only poetry.lock

The auto variant is not suitable for libraries IMO, because we want to keep a loose requirement. Otherwise, we would frequently force users to upgrade to newer versions of our dependencies.
The lockfile-only variant should work: it will allow us to install a pinned version for CI, while keeping the requirement for downstream users loose.

A general note on poetry is that it does both package building and management of (development) virtual environments. There's a way to use an existing venv, but it's not the default flow.
It also does not support pip install -e, but poetry install provides the same functionality. I have not tested compatibility with conda environments.
Note: As soon as the wheel is built, this all does not matter; the wheel format is independent of the packaging tool.

If we want to include dependabot in the template (my opinion: probably yes), I would suggest using poetry with the lockfile-only option in dependabot.

@akaszynski
Copy link
Contributor

I'm not deadset on flit or poetry, but I do like how much more popular poetry is and how it has better support for dependabot. You can also avoid installing the root package and install extras only with --no-root --extras testing.

Let's go with poetry unless there are any objections.

@greschd greschd changed the title Switch packaging from setuptools to flit Switch packaging from setuptools to ~flit~ poetry Feb 11, 2022
@greschd greschd changed the title Switch packaging from setuptools to ~flit~ poetry Switch packaging from setuptools to poetry Feb 11, 2022
@greschd
Copy link
Member Author

greschd commented Feb 11, 2022

@akaszynski the poetry version (and src/ layout) version is ready for review. Switching to poetry was mostly smooth, except for having to implement this workaround to get the single-sourced version working.

@greschd greschd requested a review from akaszynski February 11, 2022 13:13
Copy link
Contributor

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Solid LGTM. Proof will be in the pudding, but I think this is a good standard to have, especially knowing the popularity of poetry.

@akaszynski akaszynski merged commit 7ea9cc0 into ansys:main Feb 11, 2022
@greschd greschd deleted the maint/switch_to_flit_packaging branch February 11, 2022 15:45
@akaszynski akaszynski mentioned this pull request Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants