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

[DO NOT MERGE, DO NOT REVIEW] Switch to poetry #2192

Closed
wants to merge 29 commits into from

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Jan 2, 2023

 - also consolidating other settings to pyproject.toml when possible
 - both envs fail when run - not clear that the failures are new since they seem unrelated to my changes
…est poetry-core.

 - poetry-core is at 1.4.0; require that to avoid risk of encountering old bugs
 - "flake8" was causing problems with pip during validate jobs, in which pip
   tried to do this:

    py37: install_package_deps> python -I -m pip install black==22.12.0 'flake8>=4.0.1; or extra == "flake8"' 'flakeheaven<4.0.0,>=3.2.1; or extra == "flake8"' 'importlib_metadata<5.0.0,>=4.2.0; python_version >= "3.7" and python_version < "3.8"' 'isodate<0.7.0,>=0.6.1' 'isort<6.0.0,>=5.10.0' mypy==0.991 'networkx<3.0.0,>=2.6.2; (python_full_version > "3.7.0" and python_version < "3.8") and extra == "networkx"' 'networkx<3.0.0,>=2.8.8; (python_version >= "3.8" and python_version < "4.0") and extra == "networkx"' 'pep8-naming<0.14.0,>=0.13.2; or extra == "flake8"' 'pyparsing<4.0.0,>=3.0.9' 'pytest-cov<5.0.0,>=4.0.0' 'pytest<8.0.0,>=7.1.3'

    and complained about:

    pip._vendor.packaging.markers.InvalidMarker: Invalid marker: 'or extra == "flake8"', parse error at 'or extra'
…ependencies

 - exception: docker:prepare task is not yet updated
@coveralls
Copy link

coveralls commented Jan 3, 2023

Coverage Status

Coverage: 90.656% (+0.002%) from 90.654% when pulling 807ad03 on aucampia:switch-to-poetry into 9b778b3 on RDFLib:main.

- `.github/workflows/validate.yaml`:
  - Cache the default `XDG_CACHE_HOME` instead of messing with
    `PIP_CACHE_DIR` and the variable. This simplifies caching quite a bit
    and also ensures caching works for poetry.
  - Install poetry for `extra-tasks`, so that poetry install works correctly.
  - Get an auth token in `extra-tasks` so we don't get hit by rate limiting.
- `docs/conf.py`:
  - Disable nit picking on pre-python 3.7 as this causes problems with older sphinx.
- `docs/developers.rst` and `docs/docs.rst`:
  - Incorporate poetry into instructions.
- `docker-compose.yml`:
  - Set `POETRY_VIRTUALENVS_IN_PROJECT=1` so that the `.venv` goes into a docker
    volume instead of going to emphemeral storage.
- `Dockerfile.devcontainer`
  - Don't install dependencies globally. With poetry, everyone should use
    poetry, and when using poetry the global python environment should not be
    used, so if people use the devcontainer correctly the global dependencies
    should have no effect.
- `poetry.lock`
  - Ran `poetry lock` after changes to `pyproject.toml`, most notable change is
    the stuff added to the dev Poetry dependency group is no longer optional.
- `pyproject.toml`:
  - Simplified the dependency constrains for `networkx` as this was causing
    problems with tox, as tox was running pip install with
    `'networkx<3.0.0,>=2.6.2; (python_version >= "3.7" and python_version <
    "3.8") and extra == "networkx"' 'networkx<3.0.0,>=2.8.8; (python_version >=
    "3.8" and python_version < "4.0") and extra == "networkx"'` and the `extra
    == "networkx"` here would never be true.
  - Added a `dev` poetry dependendcy group with everything that is needed for a
    sane development environment so that `poetry install` gets you a sane dev
    environment.
- `Taskfile.yml`:
  - Changed the commands so that everything works as before but now instead runs things with poetry.
  - Removed the legacy venv handling and replaced it with poetry equivalents.
- `tox.ini`:
  - Don't set nitpicky (`-n`) on the command line as it is being set
    conditionally on python version in `docs/conf.py` now.
@aucampia aucampia requested review from a team and removed request for a team January 3, 2023 23:20
tox.ini Outdated
@@ -16,7 +16,7 @@ setenv =
extensive: POETRY_ARGS_extensive = --extras=html --extras=berkeleydb --extras=networkx
lxml: POETRY_ARGS_lxml = --extras=lxml
commands_pre =
py3{7,8,9,10,11}: poetry --version
py3{7,8,9,10,11}: poetry lock --check
py3{7,8,9,10,11}: poetry install --no-root --only=main --only=dev --only=tests --extras=html {env:POETRY_ARGS_docs:} {env:POETRY_ARGS_extensive:} {env:POETRY_ARGS_lxml:} --sync
Copy link
Contributor

Choose a reason for hiding this comment

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

There may be an issue with using "poetry install" here instead of "poetry export" followed by "pip install" using the exported requirements. The problem is that "poetry install" will install into a poetry-controlled venv - but tox creates/uses its own virtual environments. Even if the former method works to some extent, it'll start to be problematic with tox -p, since the various poetry install commands will all be installing into the same venv. That causes much pain, as I've learned.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, instead of the multiple specifications of "--only", which could be confusing, you could also do --with=dev --with=tests - main will be included by default.

Copy link
Member Author

@aucampia aucampia Jan 10, 2023

Choose a reason for hiding this comment

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

poetry seems to be installing into the virtual environment indicated by VIRTUAL_ENV - so for tox -e py37 it is using .tox/py37: https://gist.github.com/aucampia/acc2fb08b0f577a7592042e1021f8910#file-output-txt-L69-L83

I will commit shortly with more debugging output and link to the build output there, but as far as I can tell it is fine.

It was surprising to me that it worked when I noticed it but I think it makes some sense. See also:

This seems to be a consequence of:

This is also how poetry recommends using it: https://python-poetry.org/docs/faq/#usecase-2

I asked about documentation about this here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aucampia aucampia force-pushed the switch-to-poetry branch 2 times, most recently from 752f61a to a5ecf45 Compare January 10, 2023 22:54
This is mainly so that poetry export is sane, but also because the extras should
not be polluted with things that peope who use the wheel don't want or use.

Other changes:

- GitHub Actions:
  - Use `abatilo/actions-poetry` instead of `snok/install-poetry` as snok does
    not work well on Windows.
  - Install poetry before running tox so tox can call poetry.
  - Fix caching on Windows and MacOS
- Add requirements files for poetry so its versions is managed by dependabot and
  so that the same version can be used in multiple contexts.
- Add requirements file for Sphinx version used in readthedocs.
- The problem with nitpicky was related to sphinx 4, not to python 3.7. So
  nitpicky is now disabled on Sphinx <= 5.
- `test/test_literal/test_xmlliterals.py`: Only run html5lib test when html5lib
  is present.
- Check poetry and poetry.lock from `.pre-commit-config.yaml`.
- Changed `.readthedocs.yaml` to use poetry.
- Added an `extra` for lxml.
Move dev dependencies from extras into poetry dep groups
@aucampia aucampia force-pushed the switch-to-poetry branch 2 times, most recently from bc183bd to bf6ca06 Compare January 12, 2023 21:46
This adds a tox environment and GHA test matrix entry for testing the
lowest allowed versions of dependencies.
jclerman and others added 5 commits January 12, 2023 23:59
test minimum versions and decrease the lower bounds for dependencies to earliest versions that pass
…mmands inside the devcontainer with docker volumes from the host
This should avoid interference between different containers and the host
environment.
jclerman and others added 3 commits January 15, 2023 19:40
change `Black` -> `isort` in isort config.
@aucampia
Copy link
Member Author

Closing this as it has served its purpose.

@aucampia aucampia closed this Jan 18, 2023
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

4 participants