Skip to content

Add poetry dependency manager and package builder #368

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

Closed
wants to merge 29 commits into from

Conversation

tintin10q
Copy link

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:

Switching to poetry would simplify dependency and venv management, allowing new developers to get started with just two commands regardless of platform.
Ideally be able to run poetry install then run poetry run python tagstudio\tag_studio.py and have the project just work.

I improved it to the point where you can get started with just this:

poetry install
poetry run tagstudio

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

  • The CliDriver currently has many errors in it. For this reason I decided to not include it in the published package. That means the code is not there when you install it. To make that work I made each driver only be imported when you actually use it using the --ui option. So that means that --ui cli will not work because the code is not there. But it was not going to work anyways because the cli code currently does not work in the first place.
  • To make the publishing work I had to make tagstudio/resources an importable package otherwise I could not include it in the wheel.
  • The maintainers field in pyproject.toml is currently empty. But at some point that should probably be filled with the people that contributed code here

Let me know if you want me to change anything :)

@Sigmanificient
Copy link

Sigmanificient commented Aug 22, 2024

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"

@CyanVoxel CyanVoxel added Type: Installation Installing, building, and/or launching the program Type: CI Continuous Integration / workflows labels Aug 22, 2024
@CyanVoxel
Copy link
Member

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 [tool.poetry.dependencies].

@tintin10q
Copy link
Author

tintin10q commented Aug 23, 2024

@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?

@CyanVoxel
Copy link
Member

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?

Yes, the intention is to support 3.11 as well.

Do you want me to also publish a version to pypi with this version number?

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.

@tintin10q
Copy link
Author

@CyanVoxel ok I made the changes you requested.

Copy link

@Sigmanificient Sigmanificient left a 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

@tintin10q
Copy link
Author

tintin10q commented Aug 26, 2024

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.

🯆 poetry install
real	0m18,636s
user	0m18,127s
sys	0m1,521s
🯆 time uv pip sync requirements.txt

real	0m18,684s
user	0m5,166s
sys	0m2,593s

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:

🯆 time uv pip sync requirements.txt

real	0m0,039s
user	0m0,032s
sys	0m0,046s
$ time poetry instal

real	0m0,995s
user	0m0,917s
sys	0m0,078s

Adding a dependency:

🯆 time poetry add tqdm

real	0m2,273s
user	0m1,929s
sys	0m0,114s
🯆 time uv add tqdm 

real	0m0,215s
user	0m0,101s
sys	0m0,036s

The initial install which is the most important has no difference. Besides that uv is unsurprisingly a bit faster.
Fast enough to disregard the work here and redo it? That is for @CyanVoxel to decide.

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.

Copy link

@eivl eivl left a 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:
Copy link

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.

Copy link
Author

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

Copy link

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.

Copy link
Author

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.

Copy link

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.

Copy link
Author

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?

Copy link

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.

Copy link
Author

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"
Copy link

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

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?

Comment on lines 10 to 12
sys.path.insert(
0, os.path.abspath(os.path.join(os.path.dirname(__file__)))
) # add this so that `poetry run tagstudio` works
Copy link

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

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.

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

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

@Familex Familex mentioned this pull request Aug 31, 2024
@zierf
Copy link
Contributor

zierf commented Sep 1, 2024

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:
"Please note: this wheel is an alias to other two wheels PySide6_Essentials and PySide6_Addons, which contains a predefined list of Qt Modules."

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 tagstudio/__init__.py should be enough to continue supporting the previous python ./tagstudio/tag_studio.py and poetry run tagstudio at the same time.

The following content in tagstudio/__init__.py should allow importing local modules. See the reference in the comment for an explanation.

# 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 __init__.py spread across the whole project.

An alternative would be using explicit (via . and ..) or absolute imports.

Explicit imports example for tagstudio/src/qt/ts_qt.py
from ..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.py
from 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 __init__.py would no longer be necessary.
Additional installed modules would not require any changes to the import lines, only those from the local project.

Changing the imports in the long run might be a good idea anyway, without having to rely on the workaround via __init__.py for current Python versions and tooling in the long term.

Calling the program via Python would then change from python ./tagstudio/tag_studio.py to python -m tagstudio.tag_studio, calling the application as a module.

nix run and poetry run tagstudio would be called like before.


And at least for packaging in Nix, the following packages block in the pyproject.toml would have been enough for me. It even works completely without it, which is why I left it deactivated again.

# packages = [
#   { include = "tagstudio", from = "." },
# ]

Since it is only this one module and the pyproject.toml is located directly in the project, it should work largely out of the box.


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 nix run github:zierf/TagStudio/poetry2nix (and I am no longer in a devShell!) or let it install directly into my system, I get the following error.

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.

@tintin10q
Copy link
Author

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 again, that is not the point of this pr. I just want to get poetry working. Another pr can try to change the imports.
I didn't want to do this because it would mean that this pr would touch much more files.

@tintin10q
Copy link
Author

It is therefore worth trying out whether the packaged application also works on another system without a QT environment.

But without QT it won't work anyways right?

@jack-mil
Copy link

jack-mil commented Sep 2, 2024

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.
requirements.txt has fallen out of favor for all but the simplest of scripts and one-off code. For a full python application, all dependencies and versions, as well as the definitive application version, and all package metadata should be in the pyproject.toml.

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 pip install -e . into any virtual environment of their choice (managed by their editor/ide, uv, venv, virtualenv, pipx, etc, etc).

Here is the guide on setuptools
https://setuptools.pypa.io/en/latest/userguide/quickstart.html

@tintin10q
Copy link
Author

tintin10q commented Sep 12, 2024

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.

@seakrueger
Copy link
Collaborator

seakrueger commented Sep 12, 2024

I would also opt for not having a requirement.txt at all if that was agreed upon. But 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.

@tintin10q
Copy link
Author

https://xkcd.com/1172/

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.

@Sapein
Copy link

Sapein commented Sep 20, 2024

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.

@CyanVoxel CyanVoxel removed the Priority: High An important issue requiring attention label Nov 15, 2024
@zierf
Copy link
Contributor

zierf commented Mar 2, 2025

Will this change be implemented? This pull request actually seemed to be finished already. According to the checks, there is only something left in the linter (the logs have already been cleaned up).

If this is merged, Issue #200 could also be implemented. At least after I have incorporated the more than 238 commits that have been added in the meantime into my fork. 😁

Without this PR and Poetry, poetry2nix will not be able to create an executable version for NixOS.

@tintin10q
Copy link
Author

tintin10q commented Mar 3, 2025

At the time there was a bit too much bike sheading and people also wanting to update dependencies in this pr so I just gave up.

I can rebase and run the linter.

# Conflicts:
#	CONTRIBUTING.md
#	pyproject.toml
#	tagstudio/tag_studio.py
…ow a plugin. Just prepend the program you want to run with poetry run.
@tintin10q
Copy link
Author

tintin10q commented Mar 3, 2025

Ok I merged main and updated the dependencies to the current ones. I fixed the formatting thing. poetry run tagstudio works for me so it can now be merged. The test pass, the type checker doesn't find problems.

@xarvex
Copy link
Member

xarvex commented Mar 5, 2025

I've been reading this thread up and down, and there doesn't seem to be a compelling reason to use Poetry. PyPi publishing does not matter for us as an application, and using it for the sake of Nix is not enough to warrant an overhaul that affects everyone. Even with that aside, poetry2nix has been in an unmaintained status since November 2024. There are some people doing the occasional pull, but it is not a project I would rely upon; it seems to be treated as something receiving support for the sake of eventually transitioning away.

If we are to switch dependency manager in the future, it will most likely be to uv, stated above, in which I have done some tinkering with uv2nix as an experiment (even though Nix support shouldn't be the reason, it is being considered).
Please, do not take this as guidance to make a PR adding in uv. Simply put, there are better things to address at the moment for the project, and a change in package manager would be done at a “quieter” time. When the time comes, I won't mind putting motion into it.

Last note about Nix: I have been revamping the flake and modules as of recently, currently with a shell using a virtualenv, redone with some old dependencies trimmed away. It also accomplishes only wrapping the Python interpreter with LD_LIBRARY_PATH, something I failed to do with the old devenv implemtation (yes, we are switching away from devenv, it made sense at the time). My current task now is packaging TagStudio and the relevant Python dependencies. I will make an issue to track all of this in the morning, so some scattered Nix issues can be “bundled” in one place.

@tintin10q
Copy link
Author

I dont know or really care about nix. I fail to see how merging this pr would affect everyone. If you want to keep using requirements.txt then you can. By merging this pr you just give devs the option to use poetry.

@tintin10q
Copy link
Author

Also I think pypi publising does matter. Why wouldn't you make tagstudio pip installable? It's already setup and it would make tagstudio available to everyone with pip installed which is a lot of people. The name on pypi has been claimed as I was the one who orginally claimed it for CyanVoxel.

@CyanVoxel
Copy link
Member

#844, which has just been merged, has just significantly refactored the project layout to better align with the "src-layout" structure that was intended from the beginning of this project. Along with this layout change has come several other project-level changes that have been made easier thanks to this corrected layout.

This includes moving all dependencies to the pyproject.toml file and removing the requirements.txt and requirements-dev.txt files. This was done in a dependency manager agnostic way, which was one of my biggest issues with the approach here. I did not want the overhead of having to maintain separate ways of installing packages that included the old method plus a poetry-specific method with all of the numerous [tool.poetry] entries in the pyproject.toml. I did not wish to use Poetry as I found it more cumbersome to setup and interface with than a bare venv or an alternative dependency manager such as uv. #844 however circumvents all of this by allowing the dependencies to be declared once and are made installable with either a classic venv + pip approach, uv, or Poetry 2.0 since that release fixes a lot of my gripes with the large amount of specialized configuration. Now that Poetry supports PEP 621 I believe everyone should be able to use whichever dependency management solution they prefer without any undue specialized maintenance required on my part.

I believe these changes satisfy the original request in #365 and render this PR as no longer necessary. Most of, if not all the goals set out by this PR should now be accomplishable via the changes in #844. For reasons mentioned previously and reiterated now, I do not plan on supporting Poetry 1.x as it would be too cumbersome and increasingly unnecessary as people upgrade to 2.0. PyPI may be explored in the future, but is not something we're sure of or eager to do at the moment.

Lastly, the installation documentation has been greatly revamped and the setup instructions that were previously in the CONTRIBUTING.md have now been now mostly transferred over to the docs site. This includes instructions for various dependency managers, including Poetry.

Please let me know if everything has been addressed and this PR can be safely closed. If you have any requests that weren't addressed by either #844 or related to my comments about increased maintenance overhead, then they are probably best served in a new feature request. If you notice any mistakes in our updated documentation, especially related to Poetry, then you are more than welcome to submit a new PR with any corrections.

@tintin10q
Copy link
Author

Sounds good 👍

By now I have actually also moved to uv after learning about it from this pr.
I would not forget about pypi publishing. Having tagstudio pip installable with pip install tagstudio is nice. I made #851 to remember.

@tintin10q tintin10q closed this Mar 10, 2025
@zierf
Copy link
Contributor

zierf commented Mar 11, 2025

PyPi publishing does not matter for us as an application, and using it for the sake of Nix is not enough to warrant an overhaul that affects everyone.

This PR existed before Nix could be packaged. At the same time, Poetry2Nix seemed to be a good basis for packaging under Nix. It would have just fit together very well.

At the same time, it would have also solved Issue #200 and #512 directly. Poetry2Nix would also have made it possible to not use the rigid pre-built Nix packages, but also to use the packages defined in the pyproject.toml file.

Even with that aside, poetry2nix has been in an unmaintained status since November 2024.

Interesting, good to know, thanks for the info. At that time, it was not yet outdated, a suitable PR could have been launched in October. That's why I asked the question here whether this will be merged or discarded.

#844, which has just been merged, has just significantly refactored the project layout to better align with the "src-layout" structure that was intended from the beginning of this project. […] This includes moving all dependencies to the pyproject.toml file and removing the requirements.txt and requirements-dev.txt files. This was done in a dependency manager agnostic way, which was one of my biggest issues with the approach here.

Sounds good.

I believe these changes satisfy the original request in #365 and render this PR as no longer necessary. Most of, if not all the goals set out by this PR should now be accomplishable via the changes in #844.

What was missing for users was a simple way to install the program quickly and easily via their config or to run a flake directly from GitHub via the CLI.

For reasons mentioned previously and reiterated now, I do not plan on supporting Poetry 1.x as it would be too cumbersome and increasingly unnecessary as people upgrade to 2.0.

I didn't know that changes to the Flake had already been planned/implemented and I finally wanted to provide a way to install/run the program directly. Depending on this PR, that would have been either with Poetry or I would have had to look for an alternative. But since the Flake has already been revised, that at least means less work for me. 😀

Lastly, the installation documentation has been greatly revamped and the setup instructions that were previously in the CONTRIBUTING.md have now been now mostly transferred over to the docs site.

I think the documentation turned out really well! Big compliment for that. 👍

Please let me know if everything has been addressed and this PR can be safely closed.

When I took a quick look, I didn't find any serious errors. The .desktop file is unfortunately still missing in the flake, but I think I'll address that directly in issues #415 and #847.

When I run it via the CLI, I still get a Path does not exist. error (penultimate line), but that may be unrelated to Nix.

$> tagstudio

2025-03-11 21:59:44 [info     ] [FFMPEG] Using FFprobe location: ffprobe
2025-03-11 21:59:44 [info     ] [FFMPEG] Using FFmpeg location: ffmpeg
2025-03-11 21:59:44 [info     ] [ResourceManager] Resources Registered: count=27
2025-03-11 21:59:44 [info     ] [Config] Config File not specified, using default one filename=~/.config/TagStudio/TagStudio.ini
2025-03-11 21:59:44 [info     ] [Config] Thumbnail cache size limit: 500 MB
qt.multimedia.ffmpeg: Using Qt multimedia with FFmpeg version 7.1 GPL version 3 or later
qt.multimedia.ffmpeg: Available HW decoding frameworks:
qt.multimedia.ffmpeg:      vaapi
qt.multimedia.ffmpeg:      vdpau
qt.multimedia.ffmpeg:      vulkan
qt.multimedia.ffmpeg: Available HW encoding frameworks:
qt.multimedia.ffmpeg:      vaapi
qt.multimedia.ffmpeg:      vdpau
qt.multimedia.ffmpeg:      vulkan
2025-03-11 21:59:46 [error    ] Path does not exist.           open_path=None
2025-03-11 21:59:46 [info     ] FFmpeg found: {self.ffmpeg}, FFprobe found: {self.ffprobe}

@zierf zierf mentioned this pull request Mar 11, 2025
3 tasks
@xarvex
Copy link
Member

xarvex commented Mar 12, 2025

PyPi publishing does not matter for us as an application, and using it for the sake of Nix is not enough to warrant an overhaul that affects everyone.

This PR existed before Nix could be packaged. At the same time, Poetry2Nix seemed to be a good basis for packaging under Nix. It would have just fit together very well.

At the same time, it would have also solved Issue #200 and #512 directly. Poetry2Nix would also have made it possible to not use the rigid pre-built Nix packages, but also to use the packages defined in the pyproject.toml file.

Not specific to poetry2nix but rather Poetry as a whole that CyanVoxel mentioned, but I forgot to bring up, is that very thing around wanting the development workflow around here to be as agnostic as possible. Contributors should be able to use whatever tools they prefer. Poetry 1.X had the problem of needing dependencies stated its own way, which puts us in a spot of duplicating entries (where there is potential for drift), or “choosing” its methods.

As for the Nix equation, it would be nice to use projects defined in the pyproject.toml, I agree. I used poetry2nix in one of my own projects to sort of tinker with, and when it was unmaintained, switched its implementation to uv2nix. There is also pyproject.nix, something uv2nix uses under the hood, that I considered using directly as well. Ultimately, I went with the Nix packages because they provide some benefits with their rigidity. In terms of deduplication, if a Nix user use programs in their configuration alongside TagStudio that might share dependencies, that one package is “shared” in the Nix store. It also means the nixpkgs cache is used. Lastly, and speaking of nixpkgs, TagStudio is something I wish to upstream, and that wouldn't be possible if providing a package with one of these schemas. Sure, a different package could be provided in the flake and in nixpkgs, but that creates additional maintenance burden.

Even with that aside, poetry2nix has been in an unmaintained status since November 2024.

Interesting, good to know, thanks for the info. At that time, it was not yet outdated, a suitable PR could have been launched in October. That's why I asked the question here whether this will be merged or discarded.

You are correct in that the status of maintenance was not relevant at the time in this PR, my answer was only in the state of things now when it was bumped. Ultimately, there were many other things going on in the project that a change like this fell through the cracks. Apologies on that front.

I didn't know that changes to the Flake had already been planned/implemented and I finally wanted to provide a way to install/run the program directly. Depending on this PR, that would have been either with Poetry or I would have had to look for an alternative. But since the Flake has already been revised, that at least means less work for me. 😀

There were some rough plans I had with the flake for a long time in terms of improving it. At first, it was experience: my early work on the flake was just getting anything functional at all. Over the months and with university coming in full swing and some of my own side projects popping up, the problem of experience turned into a problem of time constraint. The whole project refactor to make it better compatible with pip, and therefore the Nix builders and tooling that use pip, was also a requirement. All to say, much of what ended up happening in the end was a little spur of the moment once the state of things was fully realized.

When I took a quick look, I didn't find any serious errors. The .desktop file is unfortunately still missing in the flake, but I think I'll address that directly in issues #415 and #847.

Yes, that still needs to be done. I left that out, as I believe it should be provided in the repository itself. I skimmed through my notifications, but I believe you left a comment on the issue agreeing with that.

When I run it via the CLI, I still get a Path does not exist. error (penultimate line), but that may be unrelated to Nix.

$> tagstudio

2025-03-11 21:59:44 [info     ] [FFMPEG] Using FFprobe location: ffprobe
2025-03-11 21:59:44 [info     ] [FFMPEG] Using FFmpeg location: ffmpeg
2025-03-11 21:59:44 [info     ] [ResourceManager] Resources Registered: count=27
2025-03-11 21:59:44 [info     ] [Config] Config File not specified, using default one filename=~/.config/TagStudio/TagStudio.ini
2025-03-11 21:59:44 [info     ] [Config] Thumbnail cache size limit: 500 MB
qt.multimedia.ffmpeg: Using Qt multimedia with FFmpeg version 7.1 GPL version 3 or later
qt.multimedia.ffmpeg: Available HW decoding frameworks:
qt.multimedia.ffmpeg:      vaapi
qt.multimedia.ffmpeg:      vdpau
qt.multimedia.ffmpeg:      vulkan
qt.multimedia.ffmpeg: Available HW encoding frameworks:
qt.multimedia.ffmpeg:      vaapi
qt.multimedia.ffmpeg:      vdpau
qt.multimedia.ffmpeg:      vulkan
2025-03-11 21:59:46 [error    ] Path does not exist.           open_path=None
2025-03-11 21:59:46 [info     ] FFmpeg found: {self.ffmpeg}, FFprobe found: {self.ffprobe}

I've gotten this too with both the devshell and the package, need to investigate it more to know where it is coming from and what the cause is; whether it is related to Nix.

I hope that provides some answers and context for you!

Edit: I've been told that indeed the path does not exist issue is not Nix specific, or even Linux specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: CI Continuous Integration / workflows Type: Installation Installing, building, and/or launching the program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants