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.

Sign up for GitHub

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

Jump to bottom

Add poetry dependency manager and package builder #368

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
+1,027 −43
Open

Add poetry dependency manager and package builder #368

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
713ea54
Added poetry stuff to pyproject.toml
tintin10q Aug 22, 2024
cbbc453
Adding src to the path fixes the console_script and poetry run tagstu…
tintin10q Aug 22, 2024
cd2dfb2
Lazy import the ui modes. Also this fixes cli not being included in t…
tintin10q Aug 22, 2024
08dd179
For poetry to work resources has to be an importable package
tintin10q Aug 22, 2024
ef51164
Add __init__.py to make tagstudio a package
tintin10q Aug 22, 2024
32e9c07
Add __init__.py to package
tintin10q Aug 22, 2024
a2f9b7c
Put packages on multiple lines
tintin10q Aug 22, 2024
bb8558c
Update tagstudio/__init__.py
tintin10q Aug 22, 2024
825af51
Shorter pypackage.toml description
tintin10q Aug 22, 2024
62df241
Added tests to package build
tintin10q Aug 22, 2024
78af477
Included tests in the release package
tintin10q Aug 22, 2024
b5fd573
Removed duplicate readme tag
tintin10q Aug 22, 2024
3cc87cf
Add poetry.lock file to vsc
tintin10q Aug 22, 2024
6f2004a
Ruff suggestions
tintin10q Aug 22, 2024
a06e90d
Update CONTRIBUTING.md with poetry instructions
tintin10q Aug 23, 2024
945b20c
Changed supported python version from just 3.12 to also 3.11
tintin10q Aug 23, 2024
f98d2f6
Bumped version to the actual version of 9.3.2
tintin10q Aug 23, 2024
15e427e
Merge branch 'main' into add-poetry
tintin10q Aug 23, 2024
3b24911
Small word repetition fix
tintin10q Aug 23, 2024
97c7337
Update pyproject.toml
tintin10q Aug 23, 2024
e8f66a9
Unupdate changes to pyproject.toml
tintin10q Aug 23, 2024
f59ad99
Added explanation text to CONTRIBUTING.md about how to add and remove…
tintin10q Aug 23, 2024
f561374
Specify where to run poetry install command.
tintin10q Aug 24, 2024
4a69f11
Merge remote-tracking branch 'origin/main' into add-poetry
tintin10q Sep 1, 2024
4b82769
Move adding the package to the import path into __init__.py
tintin10q Sep 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,51 @@ disable_error_code = ["union-attr", "annotation-unchecked", "import-untyped"]
explicit_package_bases = true
warn_unused_ignores = true
exclude = ['tests']
tintin10q marked this conversation as resolved.
Show resolved Hide resolved

[tool.poetry]
name = "tagstudio"
version = "9.1.1"
packages = [{include = "tagstudio/tag_studio.py"}, {include = "tagstudio/src/core"}, {include = "tagstudio/src/qt"}, {include = "tagstudio/resources"}, {include = "tagstudio/__init__.py"}]
tintin10q marked this conversation as resolved.
Show resolved Hide resolved

description = "TagStudio (Alpha): A User-Focused Document Management System TagStudio is a photo & file organization application with an underlying system that focuses on giving freedom and flexibility to the user. No proprietary programs or formats, no sea of sidecar files, and no complete upheaval of your filesystem structure."
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
authors = ["Travis Abendshien"]
readme = "README.md"
license = "GPL-3.0-only"
repository = "https://github.com/TagStudioDev/TagStudio"
maintainers = [] # Maybe someone can find a way to export this from GitHub?

[tool.poetry.urls]
Discord = "https://discord.gg/hRNnVKhF2G"

[tool.poetry.dependencies]
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
python = "~3.12"
humanfriendly = "10.0"
opencv_python=">=4.8.0.74,<=4.9.0.80"
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
Pillow="10.3.0"
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a comment for the rest of the pinned versions and this does not reflect a change I want to enforce, but an honest comment.

pyproject.toml in a library should focus on getting the newest version of a package that it can. This means that you should not pin versions here.

requirements.txt is the place to pin a version, as this file that the application should use. this is the file dependabot will use to check for security issues and in another scenario, this is the file that docker would use as well.

Here lies one of the issues with poetry, it is not 100% compatible with the python ecosystem, and you cant use a constraint file to enforce application versions during install. It will follow the lock file.

Micromanaging the pyproject toml files pinned versions is a little hassle, but it can ofcourse be done, this is a cost someone has to take; I'm in favor if you can. I use poetry at work so that all developers can have the same experience, at the cost of some extra work for one.

TL;DR
I don't like pinned versions in pyproject.toml, sometimes that is the only way until poetry complies with pip.

Copy link
Contributor

Choose a reason for hiding this comment

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

pyproject.toml in a library should focus on getting the newest version of a package that it can. This means that you should not pin versions here.

I think it's exact the opposite. Defining dependencies too loosely seems to lead to unforeseen problems and non-reproducible builds, especially in Python and Javascript projects. Ultimately, that is what the lock file is trying to avoid. If necessary, it can be updated quickly at any time.

requirements.txt is the place to pin a version, as this file that the application should use. this is the file dependabot will use to check for security issues and in another scenario, this is the file that docker would use as well.

Just an idea, but maybe it's possible to use a git hook to generate a requirements.txt from the pyproject.toml.
(or can an automatic build system generate a temporary one and call dependabot?)

Copy link
Author

Choose a reason for hiding this comment

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

I disagree that pyproject.toml should focus on getting the newest version of a package. The important thing is that the project works, not that the latest version is downloaded. From time to time a pr can bump versions after testing. Also in your other comment you say you actually don't like specifying ^ versions.

Why would you pin versions in requirements.txt and not in pyproject.toml? That just does not make any sense. The versions should be the same and that is why I explained in the CONTRIBUTING.md how to generate requirements.txt from pyproject.toml.

If it was possible I would not even have a requirements.txt but we have to have it of course.

Copy link
Author

Choose a reason for hiding this comment

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

I think having a git hook to update requirement.txt from the pyproject.toml is a good idea. I also thought of that but I have never set up git hooks before.

Copy link
Contributor

@zierf zierf Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The Poetry project seems to provide some git hooks.

E.g. I have seen a poetry-export git hook.

But it requires to install a poetry plugin.
Not sure if it is suitable at all or even working on NixOS systems.

I don't really have any experience with this, there may be better tools out there for creating requirements.txt files.

Maybe a temporary requirements.txt during the build would be sufficient, which would then be used for testing while in CI/CD.

Or perhaps there are better alternatives to dependabot that would work with just a pyproject.toml, the industry seems to shift towards it anyway. Unless the requirements.txt is absolutely needed for something.

Copy link
Author

Choose a reason for hiding this comment

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

Cool. But this could also be another pr?

PySide6="6.7.1"
PySide6_Addons="6.7.1"
PySide6_Essentials="6.7.1"
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
typing_extensions=">=3.10.0.0,<=4.11.0"
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
ujson=">=5.8.0,<=5.9.0"
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
numpy="1.26.4"
rawpy="0.21.0"
pillow-heif="0.16.0"
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
chardet="5.2.0"

[tool.poetry.group.dev]
optional = true
[tool.poetry.group.dev.dependencies]
ruff="0.4.2"
pre-commit="3.7.0"
pytest="8.2.0"
Pyinstaller="6.6.0"
mypy="1.10.0"
syrupy="4.6.1"


[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"

[tool.poetry.scripts]
tagstudio = "tagstudio.tag_studio:main"
1 change: 1 addition & 0 deletions tagstudio/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
__version__ = '9.1.1'
tintin10q marked this conversation as resolved.
Show resolved Hide resolved
Empty file added tagstudio/resources/__init__.py
View file
Open in desktop
Empty file.
8 changes: 6 additions & 2 deletions tagstudio/tag_studio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@

"""TagStudio launcher."""

import os, sys
sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__)))) # add this so that `poetry run tagstudio` works

from src.core.ts_core import TagStudioCore
from src.cli.ts_cli import CliDriver # type: ignore
from src.qt.ts_qt import QtDriver
Comment on lines -8 to -9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the added complicated code worth the few ms saved from doing both imports? I would argue that it is not worth it unless there is a very good reason for it. It leads to troublesome code to debug in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since if and else are the same, it could also be simplified.

    # Driver selection based on parameters.
    if args.ui and args.ui == "cli":
        from src.cli.ts_cli import CliDriver

        driver = CliDriver(core, args)
        ui_name = "CLI"
    else:
        from src.qt.ts_qt import QtDriver

        driver = QtDriver(core, args)
        ui_name = "Qt"

Copy link
Author

Choose a reason for hiding this comment

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

I disagree that this is more complicated. Do you never import things lazily? It is a great feature.
I did it like this because this allows the CLI code which does not work anyways to not be present in the package.

The cli driver can now not in the packages key in the pyproject.toml

import argparse
import traceback

Expand Down Expand Up @@ -66,12 +67,15 @@ def main():

# Driver selection based on parameters.
if args.ui and args.ui == "qt":
from src.qt.ts_qt import QtDriver
driver = QtDriver(core, args)
ui_name = "Qt"
elif args.ui and args.ui == "cli":
from src.cli.ts_cli import CliDriver
driver = CliDriver(core, args)
ui_name = "CLI"
else:
from src.qt.ts_qt import QtDriver
driver = QtDriver(core, args)
ui_name = "Qt"

Expand Down