Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

[INT-442] Poetry #375

Merged
merged 11 commits into from
Dec 3, 2021
Merged

[INT-442] Poetry #375

merged 11 commits into from
Dec 3, 2021

Conversation

bbonenfant
Copy link
Contributor

Switch from setup.py to poetry/pyproject.toml

This changes the following things:

  • No more need for a setup.py file, so it gets deleted. Building the wheel is done through poetry build and the the dependencies are tracked with pyproject.toml and poetry.lock
  • There's also no need for the MANIFEST.in file as poetry does all of that in the pyproject.toml file. There should probably be a review of this to ensure that the proper things get included in the package that were previously specified in the manifest.
  • poetry installs the virtual environment at .venv instead of venv. No way I know around this unfortunately.
  • I switched how the __version__ value gets specified. The modern way to do this is to look at the metadata of the installed package -- this functionality is built into the language in python >=3.8, but there is backport (importlib-metadata) for older versions.
    • On this point, poetry must have the version specified in the pyproject.toml so there should probably be a discussion on how this meshes with theversion.json file.
  • Had to make some tweaks to get CI working (but surprisingly fewer tweaks than I was expecting)
  • I changed the author of the package on whim to Pachyderm <[email protected]>, but I can revert to JD if that was a mistake.

Follow ons:

  • There should be a review and sanitization of the dependency pinning. In general testing dependencies should be pinned and the ^ dependency selector should be used over >/=> for package dependencies.

@CLAassistant
Copy link

CLAassistant commented Nov 30, 2021

CLA assistant check
All committers have signed the CLA.

Copy link

@albscui albscui left a comment

Choose a reason for hiding this comment

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

LGTM!

Although I don't know poetry that well, this looks really useful!

```bash
python3 -m venv venv
source venv/bin/activate
poetry install
Copy link

Choose a reason for hiding this comment

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

Does this install the dev dependencies as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! When run like this on the repo poetry will install all dependencies, including dev dependencies.

@smalyala
Copy link
Contributor

smalyala commented Dec 1, 2021

Just to confirm, if we want to bump the python-pachyderm version, we need to update it in pyproject.toml and version.json. As of right now, it seems like the python-pachyderm version in version.json is only being used in setup.py (and in test_version.py). Correct me if I'm wrong, but we can effectively remove the python-pachyderm line from version.json and instead use the version number from pyproject.toml for version tests right?

pyproject.toml Outdated
name = "python-pachyderm"
version = "7.2.0"
description = "Python Pachyderm Client"
authors = ["Pachyderm <[email protected]>"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the email [email protected] instead. @albscui thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I grabbed this from this issue on a whim. Happy to change it to the correct one

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! This integrations email is the one we're using for managing all integrations-related projects moving forward.

# Python version compatibility.
try:
# >= 3.8
import importlib.metadata as metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this read directly from pyproject.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, packages keep their official version in an associated METADATA file that these modules attempt to read (this is how pip can report the version of installed packages without a lock file).

@@ -0,0 +1,2 @@
[virtualenvs]
in-project = true
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this file do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

poetry by default tries to create all you virtual environments in your home directory, this just tells poetry to create the virtual environment in the project

Copy link
Contributor

Choose a reason for hiding this comment

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

Neat!

Copy link
Contributor

@smalyala smalyala left a comment

Choose a reason for hiding this comment

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

LGTM, just one question up above

@bbonenfant bbonenfant merged commit 25d4149 into master Dec 3, 2021
@bbonenfant bbonenfant deleted the bonenfant/INT-442-poetry branch December 3, 2021 00:14
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