-
Notifications
You must be signed in to change notification settings - Fork 381
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
Add poetry dependency manager and package builder #368
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Yohann Boniface <[email protected]>
to have __all__ Co-authored-by: Yohann Boniface <[email protected]>
Co-authored-by: Yohann Boniface <[email protected]>
Formatting diff: diff --git a/tagstudio/__init__.py b/tagstudio/__init__.py
index df6a834..feefebb 100644
--- a/tagstudio/__init__.py
+++ b/tagstudio/__init__.py
@@ -1,3 +1,3 @@
-__version__ = '9.1.1'
+__version__ = "9.1.1"
__all__ = ("__version__",)
diff --git a/tagstudio/tag_studio.py b/tagstudio/tag_studio.py
index 7213914..d557d79 100644
--- a/tagstudio/tag_studio.py
+++ b/tagstudio/tag_studio.py
@@ -5,7 +5,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
+
+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
import argparse
@@ -68,14 +71,17 @@ 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" |
The version should match the current version of the program: 9.3.2 instead of 9.1.0/9.1.1. This incorrect version is also listed on the PyPI page. I would also suggest updating the CONTRIBUTING.md with the new instructions for installing Poetry and launching TagStudio via it. I've also been meaning to update the CONTRIBUTING.md to list Python 3.11 as the minimum supported version, which would also need to be set here in |
@CyanVoxel Ok I will add instructions to CONTRIBUTING.md. At the moment it says in pyproject.toml that we only support 3.12. Do you mean you actually (want to) support 3.11 as well? Do you want me to also publish a version to pypi with this version number? |
Yes, the intention is to support 3.11 as well.
I'm honestly not sure about publishing a new version to PyPI since it wasn't my intention to have the program on there in the first place at this time. I suppose if it's going to be there though then it might as well have the correct version number. |
@CyanVoxel ok I made the changes you requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some version constraints are quite restrictive, I think we can let them be a bit more permissive.
Will have to update poetry.lock
accordingly i suppose.
Current nix derivation have
pythonRelaxDeps = [
"pillow" # 10.4.0
"pyside6" "pyside6-addons" "pyside6-essentials" # 6.7.2
"pillow-heif" # 0.17.0
"typing-extensions" # 4.12.2
"ujson" # 5.10.0
];
set, and seems to work properly
Bump opencv python and move to opencv-python Co-authored-by: Yohann Boniface <[email protected]>
… dependencies in the future with poetry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering previous statements, we are going to let deps as is, so this is LGTM
While this is also a high priority to get this squared away, I'll be prioritizing #332 over this as that adds additional dependencies and is a preexisting high priority PR |
@CyanVoxel It is up to you but I think it is probably easier to just merge this and not connect it to #332. This pull request is for the main branch and not sql-bump like #332. After merging this you could merge main into sql bump when ready and just do The point of this pull request is not to update the dependencies it is to add poetry, a system to make adding managing dependencies simpler and easier. |
#332 is also targeting
While it won't break the existing venvs we're using, it would still put these new Poetry responsibilities on #332 since it's updating the
I personally don't find Poetry necessarily easier easier to work with compared to raw venvs, especially when I've had to install new package managers for a new package just to do what the venv and #332 and it's predecessors, #190 and #187, have been constantly thwarted by the rest of the codebase updating quicker than they could keep up. The last thing I want to do is give more work to yed right now with a different dependency management system when it can be avoided by just waiting to pull this. Again if he's fine with it then it's all good, but that's my view on it as it stands. |
Alright I understand. Thanks for expanding more on this for me. I checked out the latest commit of #332 and if @yedpodtrzitko is fine with merging this pr earlier then here are the commands you would need to issue if #332 were to be merged with the dependencies it has right now: poetry add structlog==24.2.0 SQLAlchemy==2.0.32
poetry add ruff==0.6.2 mypy==1.11.1 pytest-qt==4.4.0 --group dev This will update poetry.lock and pyproject.toml correctly. Besides this and installing poetry you can just ignore it. |
tl;dr, I have just a few questions
|
I looked at uv and it looks quite nice I have not heard of it before. I do not see the publishing feature that poetry has however. The rest of the things you mention, easy of installation, virtualenv handling etc I think are all the same. But uv claims to be a bit faster indeed. As for ease of install Poetry just recommends to install with pipx but there are also separate install scripts or you can install it with pip so I think that is the same as well. As for #293 I don't see how poetry would also not fix that. |
I dont want to disregard the effort you put into this, but why does PyPI publishing matter? Normal users will download the executable package. I dont see the use-case for installing python, setting up virtualenv just so I can install it from PyPI and run in awkward way from shell... afaik nobody runs (nor publish) desktop apps this way. So if the reason to choose Poetry (over uv) is because it has a feature which is not necessary / relevant for the project (as @CyanVoxel mentioned his stance above), then is that still the best option? |
I don't think PyPi publishing is particularly important in this case I just mentioned it as something uv doesn't have. I understand that most normal people will just download an executable. I did a rough test using the time command. Installing dependencies for the first time.
Its basically the same. I gave uv leeway here because creating the venv is a seperate command in uv (not included in the time) but with poetry that was also included in the time. Running poetry install again uv when nothing needs to happen:
Adding a dependency:
The initial install which is the most important has no difference. Besides that uv is unsurprisingly a bit faster. I personally think poetry is good and I also like the cli interface more. But I am of course biased. Also do not forget that other people also mentioned poetry independently before I made this. But of course they might not have known about uv either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When my comments are resolved I can fully approve this pr.
|
||
If you wish to launch the source version of TagStudio outside of your IDE: | ||
After installing poetry and python you can install the dependencies with the following command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im in favor of sharing one way to install a tool if there is consensus to use it. I use poetry daily, there are many things I don't like about it, but it makes the interface we use the same for all developers, no matter what platform you are on.
Currently the recommended way to install poetry is by using pipx
this is a tool that is recommended for python devs to have, but it is yet another install process to document.
Im happy write the changes needed if that is something you would like me to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me what changes you are actually suggesting here. @eivl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pipx install poetry
is the command to install poetry. the docs has multiple other ways to install it, but I'm in favor of front loading the information instead of back loading it with more links to click on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so just adding pipx install poetry
in CONTRIBUTING.md with some text?
``` | ||
|
||
Do not forget to actually commit the updated requirement.txt and pyproject.toml and poetry.lock files when you change/add dependencies. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an important detail missing from this MD file.
The explanation of what the lock file is. The troublesome command poetry update
and en explicit warning that this command updates packages and creates a new .lock file. If the PR is updating dependencies, sure, you would use this and update the .lock file.
When you add a new dependencies, poetry will use the caret symbol, I personally dislike this behaviour as it is implicit in nature. Are future dependencies going to follow the pinned nature of the repo? I don't know, but poetry will update the pyproject.toml file as it sees best. This might lead to trouble and worth a note in the readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The section you comment on explains how to add dependencies. The reader does not need to know what the poetry.lock
file is because it is updated correctly when they run poetry add <dependency>
as described. No need to run poetry update
either. I don't understand why you bring up poetry update
because I did not tell the reader to run this command.
I could add a sentence saying that it is better to pin a specific version of a dependency poetry add package==0.1.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a thing I have seen many times in projects that start using poetry. I don't mind the sections that are already added. Im saying I think this detail is missing. It has burned many people in projects I have worked on the past years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you suggest a text? Like there is not more to it then the poetry.lock pins the versions of each dependency so you get reproducible builds.
python = ">=3.11 <3.13" | ||
humanfriendly = "10.0" | ||
opencv_python=">=4.8.0.74,<=4.9.0.80" | ||
Pillow="10.3.0" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
tagstudio/tag_studio.py
Outdated
sys.path.insert( | ||
0, os.path.abspath(os.path.join(os.path.dirname(__file__))) | ||
) # add this so that `poetry run tagstudio` works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't you add an entrypoint console script? I'm not sure if this is a solution to the problem or just a solution to a xy problem. Can you elaborate about this please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my following comment or ModuleNotFoundError for poetry script for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did add an entry point to the script. But this project uses . relative imports so you need to fix that using this. Of course I could not have moved away from these relative imports. You can also have this problem when adding an extra py script in the root it is about where the working directory is. Something you can not change in the entry point definition either.
I think adding the module to the import path is an elegant solution. This way it will just always work, not just for poetry.
from src.cli.ts_cli import CliDriver # type: ignore | ||
from src.qt.ts_qt import QtDriver |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
Looking forward to this, Poetry is also currently the best tool for packaging Python into Nix packages. In my attempts I also came across a couple of things and I have also gone over this pull request a bit. Maybe I can share one or two things here. The PySide6 Python package officially states: I saw something similar again in an info about the package split into Essentials and Addons, but I can't find it anymore. PySide6 = "6.7.2"
# PySide6 includes PySide6_Essentials and PySide6_Addons
# PySide6_Addons= "6.7.2"
# PySide6_Essentials = "6.7.2" A single The following content in # allow local module files as import
# https://github.com/python-poetry/poetry/issues/3928#issuecomment-1399313433
import os, sys; sys.path.append(os.path.dirname(os.path.realpath(__file__))) No need for multiple new An alternative would be using explicit (via Explicit imports example for tagstudio/src/qt/ts_qt.pyfrom ..core.enums import SettingItems, SearchMode
from ..core.library import ItemType
from ..core.ts_core import TagStudioCore
from ..core.constants import (
COLLAGE_FOLDER_NAME,
BACKUP_FOLDER_NAME,
TS_FOLDER_NAME,
VERSION_BRANCH,
VERSION,
TAG_FAVORITE,
TAG_ARCHIVED,
)
from ..core.utils.web import strip_web_protocol
from .flowlayout import FlowLayout
from .main_window import Ui_MainWindow
from .helpers.function_iterator import FunctionIterator
from .helpers.custom_runnable import CustomRunnable
from .resource_manager import ResourceManager
from .widgets.collage_icon import CollageIconRenderer
from .widgets.panel import PanelModal
from .widgets.thumb_renderer import ThumbRenderer
from .widgets.progress import ProgressWidget
from .widgets.preview_panel import PreviewPanel
from .widgets.item_thumb import ItemThumb
from .modals.build_tag import BuildTagPanel
from .modals.tag_database import TagDatabasePanel
from .modals.file_extension import FileExtensionModal
from .modals.fix_unlinked import FixUnlinkedEntriesModal
from .modals.fix_dupes import FixDupeFilesModal
from .modals.folders_to_tags import FoldersToTagsModal Absolute imports example for tagstudio/src/qt/ts_qt.pyfrom tagstudio.src.core.enums import SettingItems, SearchMode
from tagstudio.src.core.library import ItemType
from tagstudio.src.core.ts_core import TagStudioCore
from tagstudio.src.core.constants import (
COLLAGE_FOLDER_NAME,
BACKUP_FOLDER_NAME,
TS_FOLDER_NAME,
VERSION_BRANCH,
VERSION,
TAG_FAVORITE,
TAG_ARCHIVED,
)
from tagstudio.src.core.utils.web import strip_web_protocol
from tagstudio.src.qt.flowlayout import FlowLayout
from tagstudio.src.qt.main_window import Ui_MainWindow
from tagstudio.src.qt.helpers.function_iterator import FunctionIterator
from tagstudio.src.qt.helpers.custom_runnable import CustomRunnable
from tagstudio.src.qt.resource_manager import ResourceManager
from tagstudio.src.qt.widgets.collage_icon import CollageIconRenderer
from tagstudio.src.qt.widgets.panel import PanelModal
from tagstudio.src.qt.widgets.thumb_renderer import ThumbRenderer
from tagstudio.src.qt.widgets.progress import ProgressWidget
from tagstudio.src.qt.widgets.preview_panel import PreviewPanel
from tagstudio.src.qt.widgets.item_thumb import ItemThumb
from tagstudio.src.qt.modals.build_tag import BuildTagPanel
from tagstudio.src.qt.modals.tag_database import TagDatabasePanel
from tagstudio.src.qt.modals.file_extension import FileExtensionModal
from tagstudio.src.qt.modals.fix_unlinked import FixUnlinkedEntriesModal
from tagstudio.src.qt.modals.fix_dupes import FixDupeFilesModal
from tagstudio.src.qt.modals.folders_to_tags import FoldersToTagsModal After changing the imports, the above code in Changing the imports in the long run might be a good idea anyway, without having to rely on the workaround via Calling the program via Python would then change from
And at least for packaging in Nix, the following # packages = [
# { include = "tagstudio", from = "." },
# ] Since it is only this one module and the Nix Flakes are (usually) pure by nature. If I run the application in a devShell, it starts fine. However, if I run it directly from GitHub with warning: Git tree '/home/zierf/workspaces/Python/TagStudio' is dirty
Traceback (most recent call last):
File "/nix/store/k98sinf1gjvbyld65nmqm5zly4lyggrw-python3.12-tagstudio-9.3.2/bin/..tagstudio-wrapped-wrapped", line 6, in <module>
from tagstudio.tag_studio import main
File "/nix/store/k98sinf1gjvbyld65nmqm5zly4lyggrw-python3.12-tagstudio-9.3.2/lib/python3.12/site-packages/tagstudio/tag_studio.py", line 8, in <module>
from .src.cli.ts_cli import CliDriver # type: ignore
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/k98sinf1gjvbyld65nmqm5zly4lyggrw-python3.12-tagstudio-9.3.2/lib/python3.12/site-packages/tagstudio/src/cli/ts_cli.py", line 32, in <module>
from ..qt.helpers.file_opener import open_file
File "/nix/store/k98sinf1gjvbyld65nmqm5zly4lyggrw-python3.12-tagstudio-9.3.2/lib/python3.12/site-packages/tagstudio/src/qt/helpers/file_opener.py", line 13, in <module>
from PySide6.QtWidgets import QLabel
ModuleNotFoundError: No module named 'PySide6.QtWidgets' It is therefore worth trying out whether the packaged application also works on another system without a QT environment. |
Thanks for your comments! As for specific versions, I created this pr to add poetry not to change versions of dependencies. I suggest you make a new pr to change the PySide6 dependency. But probably after this has merged. I don't really see the problem of having multiple init.py. But moving the code that adds the import path there is a good idea! Iirc @CyanVoxel mentioned he did not want to do local imports or prepend the package name. It also says it here: https://github.com/TagStudioDev/TagStudio/blob/main/CONTRIBUTING.md#implementations I agree though having dots or module name imports would be better. I would suggest using the dots. Al tough it does look a little weird if you see it for the first time. But in my opinion: yes it would be better to change the imports. |
But without QT it won't work anyways right? |
Just wanted to chime in as a python developer that has built and maintained several python packages and applications. A pyproject.toml should definitely be the place to list dependencies, as it's the current standard for all python packages. This is preferred over a requirements.txt and manual invocation of pip. Even if end users download the binary releases as a Windows desktop application and this is never published to pypi, for developers it's good to have a python package definition. Poetry can be useful, but setuptools is also a perfectly capable build system, and can be simpler and easier to use for some projects. It also has the advantage that developers aren't forced to use a specific tool. They can choose to Here is the guide on setuptools |
I would also opt for not having a requirement.txt at all if that was agreed upon. I like poetry because who likes making virtual enviroments. |
I would prefer to keep the requirement*.txt files. I personally don't see the need for an external dependency manager, and would rather keep managing my own virtual envs as it works better for my workflows. |
But yeah no reason to get rid of the requirements.txt, but a dependency manager that makes the venv and everything for you is also nice. |
Why keep the requirements.txt around? You don't need it anymore even for setuptools, which has support for using pyproject.toml for all dependencies. |
What is this?
This pull request adds poetry support. Poetry provides an easy way to install dependencies and to publish.
This change was first suggested by #365 but I actually started working on it just before that issue was created.
As was stated in that issue:
I improved it to the point where you can get started with just this:
That tagstudio run script also sets up a console_script. That basically allows you to start tagstudio by just calling
tagstudio
in a shell after installing with pip.Using poetry you can also easily publish the package to pypi so that one can actually install using pip with:
poetry publish --username __token__ --password <api_token_generated_on_pypi>
link to poetry website
Setting up publishing infrastructure
On this discord it was suggested to make the publishing infrastructure part of the same pr. Currently if you want to publish just adjust the version number and issue the comand above.
However, I think its nice to make a github action so that it publishes a new release when you make a github release.
I found a pretty good looking github workflow here
I saw that on pypi that you can add github as a Trusted Publisher here. I assume this works by adding the workflow and then you can simply point pypi to that yml file.
But honestly I have not set that up before. For this reason I think this should just be a separate pr. Also because do we even want to use poetry for this? I think its a good idea because then dependency management and publishing is just one tool.
I suggest to just merge this and setup the automatic release to pypi (and apt) upload when you actually want to do a release.
Also do not forget that pypi has a test version where you can test out uploads without wasting a version number on the official one.
Things to know
tagstudio/resources
an importable package otherwise I could not include it in the wheel.Let me know if you want me to change anything :)