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

Install extra for "dev" dependencies #1057

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Install extra for "dev" dependencies #1057

wants to merge 1 commit into from

Conversation

nixjdm
Copy link
Member

@nixjdm nixjdm commented Aug 31, 2022

This PR adds an extra install dependency to make development a little easier. Since the move to tox, the old test extra went away. We now also have pre-commit in the mix. This adds all of the testing and linting tools, and ipdb, to the dev dependencies, except for what pre-commit runs, so people like myself can get up and running more quickly.

I specifically included all of the pytest deps like before tox, because I often prefer to run pytest directly while developing something. Tox can add headache, especially if I'm trying to put a breakpoint in a test script. Now pytest and tox are both ready to go, so I can use either.

@dairiki
Copy link
Contributor

dairiki commented Aug 31, 2022

In other projects, I've taken to using the --devenv parameter of tox for generating dev virtualenvs. (It generates a virtualenv, with dependencies installed as specified for a specific testenv, and the current project installed in editable mode.)
That keeps all the dev dependency configuration in one place (the tox.ini.)

It's a little hokey, since often there's not a single testenv that depends on all the things we want in our dev virtualenv, so I often end up adding a bogus devenv testenv to the tox.ini that looks something like:

[testenv:dev]
# Not an actual test testenv, this is here so that you can generate a development virtualenv by doing:
#
#     tox -e dev --devenv venv
#
skip_install = true
deps =
    {[testenv]deps}
    {[testenv:lint]deps}

If we do reinstate dev or test extras, I hate to duplicate the config between those and the tox.ini deps. They should be one place or the other, lest they get out of sync.

Tox does now support an extras setting to install extras, so it's no longer necessary to resort to the hacky deps = .[test], which sort of installs the main package twice — one can just do extras = test.

```shell
git clone https://github.com/lektor/lektor
cd lektor
virtualenv venv
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're fixing these instructions, this should probably be updated to python -m venv venv (since the venv is in py3's standard library, whereas virtualenv is an external dependency.)


Or, alternatively, we could use tox's --devenv option. E.g., right now, if you do

tox --devenv venv

that will create a virtual environment with all the dependencies for the default [testenv] installed (i.e. pytest and friends), and Lektor installed in editable mode, all in one go.

If we want to support installing the kitchen sink — more than what is required by any particularly testenv —we could include a "dev" testenv in the tox.ini (which doesn't actually do any testing)

[testenv:dev]
skip_install = true
commands =
deps =
    # pull in dependencies declared in other testenvs
    {[testenv]deps}
    {[testenv:lint]deps}
    # add some dependencies of our own
    pre-commit

so that

tox -e dev --devenv venv

will create a virtual environment with everything installed and ready to go.

@nixjdm
Copy link
Member Author

nixjdm commented Sep 2, 2022

I didn't know tox could create for you a venv - that's cool. Thank you for showing me that. A couple little issues I have with the this approach though:

  • tox itself is a development dependency, but you'd have to install it separately, first, if the final dev env needs created with and managed by tox
  • the dev env has to use tox, or else you install these dev deps by hand. Whereas before, using a venv wasn't really even required. Someone could pip install in their userspace, in a conda env, pyenv-virtualenv, etc., so there's less freedom with tox.

I agree that I'd rather have the dependencies listed in a single place. And if some folks like you are familiar with venvs through tox, it would be nice to enable that as well. I'm wondering if we can't have tox reference the dev deps in the setup.cfg, or both use a requirements file (though I'd like to not add another top-level file), or something like that. Searching a bit - I find https://github.com/hypothesis/tox-recreate. Though that's a brand new project, by the looks of it.

@dairiki
Copy link
Contributor

dairiki commented Sep 3, 2022

A few random comments, as I see things:

There may not be one ideal canonical dev environment

We are trying to specify a canonical development environment here. As you point out, we (developers) all use different tools and have different workflows — in addition to managing our dev virtualenvs in different ways, we use different editors/IDEs, etc. I'm not sure the idea of a canonical set of dev dependencies is all that well defined — it seems there will always be a lot of personal preference involved.

As far as specifying the project's tests — the dependencies required to run them, and how and which tests to run, tox.ini is the authoritative specifier1.
As far as code style and linting for the project, .pre-commit-config.yaml is the source of truth2.

For everything else, it all kind of depends on the individual developer's choice of workflow. Does pylint need to be installed in the dev venv? It depends. Does ipdb need to be installed? That almost certainly is a point of developer preference.

We're developers. We can (and will) figure these things out for ourselves! ;-)

Extras_require really wasn't meant for dev dependencies

Extras are part of the distribution metadata. But the dev "requirements" don't add any functionality to the packaged distribution (as installed from PyPI), so I don't think they belong there.

When installing Lektor from PyPI you can do:

pip install Lektor[test]==3.2.3

and pytest, pre-commit, and pylint will be installed. But the Lektor tests are not installed, so none of those do much good. It's confusing and weird to users of the distribution, and not the original intent, I think, of what extras were meant for. All of those developer tools are really only useful when working with Lektor source code (e.g. from git), not when using the Lektor distribution.
(Contrast that to the Lektor[ipython] extra. That is a bone fide extra in that installing it adds optional extra functionality to the lektor dev shell sub-command.)

Tox.ini specifies multiple sets of deps, this makes moving their declaration to extras problematic

There are a number of different testenvs defined in our tox.ini. Some have different dependencies than others. E.g. the "lint" testenv (alone) wants pylint installed.

Moving these dependencies to extras would require either:

  • introducing multiple different extras for the various sets of dependencies
  • using one extra which is the superset of all the various testenv dependency sets

Neither of those options seems ideal.

Footnotes

  1. Along with various bits of tool configuration in pyproject.toml.

  2. Except that, because pylint and pre-commit don't seem to play well with each other, we have our pylint acceptance testing configured as a tox testenv.

@nixjdm
Copy link
Member Author

nixjdm commented Sep 7, 2022

I agree that dev extras probably wasn't the intended use, though I'm skeptical it causes much confusion. It seems fairly common, as does a test extra without shipping corresponding tests.

I also don't really see it as an issue that the list of dev dependencies isn't canonical. To me, it should be a convenient start that shouldn't cause any headaches, and I'd be open to adding a little to the list, beyond e.g. ipdb. Dev deps are a fuzzy thing, but helps most of the time. I'd resist adding dependencies that were for some reason problematic; e.g. slow to install, forcing onerous version constraints on other things, etc.).

The motivation is reduced effort to get a developer started and allowing greater flexibility in workflow, where that's not too burdensome on this team.

Other Options

All that said, I'm open to other solutions besides an extras_require. Here's the three top options so far IMO

Tox-provided venv

This is as you suggested above. I don't think this checks all of the boxes, but it is still improvement. If nothing else passes muster, I'd at least like to see some changes to support this approach.

Requirements file(s)

How about a requirements file(s) that the tox.ini uses? I'm not wild about extra top-level items (file or dir), but it would work.

Poetry

There's also talk recently (#1055) about possibly migrating Lektor to something like Poetry. If we did that, I think we could have a standard [tool.poetry.dev-dependencies], and tox use it too, in one of a couple ways. Between the various tox usecases in their faq, it seems like we can get what I'm after, keep a fairly clean top-level dir, not list deps in multiple locations, and not pollute extras_require. So far as I can tell this seems almost ideal. We'd just also have to adopt Poetry.

Summary

I wouldn't use this as the reason to adopt Poetry. Merely, it's a thing I'd like to do if we did adopt it.

In the meantime, what seems like the best change so far? Are there still other options?

I appreciate the back and forth by the way. Thank you for the constructive feedback.

@dairiki
Copy link
Contributor

dairiki commented Sep 8, 2022

I also don't really see it as an issue that the list of dev dependencies isn't canonical.

If I understand correctly, you're saying that you wouldn't mind if Lektor[dev] declares a different set of depencies than any particular tox testenv does. (That set would nominally be a strict superset of the tox deps for the main pytest testenv, but, since I haven't yet seen a particularly clean way to avoid repetition of the declaration of the deps, any changes in one would have to be manually propagated to the other.)

If you're happy with that, let's go for it. I don't think I'll be using the dev extras in my workflow, but that's okay.
(I generally use tox --devenv and then manually install whatever else I want.)


How about a requirements file(s) that the tox.ini uses? I'm not wild about extra top-level items (file or dir), but it would work.

That doesn't (if I understand correctly) get around the issue that there are several testenvs defined in tox.ini, each with (potentially) varying sets of dependencies. A single requirements.txt (like a single dev extra) doesn't really allow for that specialization between testenvs in a non-clumsy fashion.

There's also talk recently (#1055) about possibly migrating Lektor to something like Poetry. If we did that, I think we could have a standard [tool.poetry.dev-dependencies], and tox use it too, in one of a couple ways. Between the various tox usecases in their faq, it seems like we can get what I'm after, keep a fairly clean top-level dir, not list deps in multiple locations, and not pollute extras_require.

Poetry does have two different (overlapping?) sorts of optional dependencies: dependency groups, and "extras". Dependency groups appear to be Poetry-local (project-local), and so, as you say, can be used for managing one or more dependency sets without them ending up in the distribution metadata. ([tool.poetry.dev-dependencies] appears to be just a shorthand for [tool.poetry.group.dev.dependencies].)

I am still unconvinced that moving the Lektor distribution to Poetry gets us enough to be worth the extra dependency. (As noted in the discussion on #1055, at least AFAICT, Poetry's advanced dependency resolution and lock-file only affect those installing Lektor from git — so perhaps it helps devs. But none of that would propagate to the Lektor packages as installed from PyPI, the PyPI distribution still gets just the list of dependencies (and extras) as declared in the pyproject.toml file — really no different from a setuptools-based distribution.)

Poetry does provide the "^1.2.3" shorthand for specifying ">=1.2.3, <2.0.0". Not that we can't/shouldn't do that manually in our setup.cfg, but that does provide some readability.

I'm not convinced that Poetry's use of a lock-file for testing and development is very useful for us. (Users who install Lektor from PyPI don't have access to that lock-file. Shouldn't we test with the same set of dependencies — dynamic though it may be — as they get?)

If I understand Poetry's suggested usages of tox:

Usecase #1 - Poetry provides nothing here that setuptools doesn't also provide. Tox builds our distribution (using PEP517) and installs it in the testenv.

Usecase #2 - As usecase #1, but uses Poetry, in essence, to install the [dev] extras. What Poetry contributes here is that it uses its lock-file when doing this. (As noted above, it's arguable whether that is what we want.)

Usecase #3 - Poetry builds the whole testenv. It's not clear that this is what we want either.


Another Option

Another option would be just to include a list of useful dev packages in the README. We seem to agree that most of these extras are not dev requirements, but rather a matter of personal workflow and tooling preference. Just mentioning a suggested list may be enough. It's not that hard to pip install them manually. (Again, for the most part, tox.ini provides the source of truth on testing requirements, and pre-commit the source of truth on code-style requirements. Beyond that, things are a matter of personal preference.)

E.g.:

Want to develop on Lektor?

This gets you started (assuming you have Python, pip, tox, Make, node, and pre-commit installed):

$ git clone https://github.com/lektor/lektor
$ cd lektor

Create and activate a virtualenv containing the test dependences and with Lektor installed in editable mode:

$ tox --devenv venv
$ . venv/bin/activate

At this point, you may want to install a number of optional Python distributions which may prove useful during development.

pip install ipdb pylint

Build the JS and CSS used by the admin frontend:

$ make build-js

Install pre-commit hooks

$ pre-commit install

Run Lektor in dev-mode:

$ export LEKTOR_DEV=1
$ cp -r example example-project
$ lektor --project example-project server

@dairiki
Copy link
Contributor

dairiki commented Sep 10, 2022

I just stumbled across hatch which may be worth a look as an alternative to Poetry or pipenv.

It's brand new (1.0 released in April) but is an official project of PyPA. It looks to be a bit lighter-weight than Poetry, but supports the idea of multiple dev environments (matrices of them, even). From a brief look, these environments seem more configurable than in other tools. It looks like hatch can replace most of the capabilities of tox, venv, setuptools, build, twine — all with a single CLI tool.

It has some sort of plugin architecture, so perhaps we can manage to integrate things like the npm run build part of our build process into hatch's build command.

No package lock-file support yet (but I don't think Lektor — being more of a "library" than an "application" really wants that.)

It looks pretty slick to me. (Though I haven't tried using it yet.)

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

Successfully merging this pull request may close these issues.

None yet

2 participants