Convert to python module (pyproject.toml) #659

VannTen · 2022-09-19T09:11:44Z

possible followup to #658

harshad16

lgtm
just one thought , i would like to share.

harshad16 · 2022-09-20T20:18:23Z

producer.py

@@ -46,14 +43,23 @@
 from thoth.python import AIOSource
 from thoth.python.exceptions import NotFoundError

+try:


The reason the __version__ was kept in the version.py
is as we use kebechet for release, it currently only recognizes these list of files.
https://github.com/thoth-station/kebechet/blob/4c52e72a0ee0b9299b5165d77cc50c33b58f0350/kebechet/managers/version/release_triggers.py#L104

Hum.
Relevant:

Add option to automatically create new release kebechet#1062 (probably)

[3pt] Move away from setuptools and use pyproject.toml instead core#360

Do you mean kebechet require handling version in those way to handle releases ? Should'nt that be separate ?
(I don't see support for pyproject.toml, setup.cfg / others here. Would that mean that python packages using those can't use kebechet for that purpose ?)

yes, correct, there isn't support for these in kebechet
thanks for the issue, let's make kebechet more better. 👍

KPostOffice · 2022-12-07T22:47:20Z

producer.py

+try:
+    __version__ = version(__package__)
+except PackageNotFoundError:
+    __version__ = "v0.11.10"  # TODO makes this unnecessary by fixing users of the package


Have this default to dev? I think local development is the most likely cause of version not being found.

KPostOffice · 2022-12-07T22:49:27Z

producer.py

+__service_version__ = (
+    f"{__version__}+"
+    + ".".join(version(f"thoth-{p}") for p in ["storages", "common", "messaging"])
+    + f".python.{__python__version__}"


I think python should be added to the above list as the python_version is actually the thoth.python version.

harshad16

On reviewing,
its seems, i missed earlier that this component is not module.
we don't need the project in pyproject.toml
and can keep the version.py for this component.
as it would be released as container image.

harshad16 · 2023-01-18T22:24:59Z

pyproject.toml

+[project]
+name = "package-releases-job"
+description = "Check packages updates on Python package indexes"
+readme = "README.rst"
+requires-python = ">=3.8"
+license = {file = "LICENSE"}
+dynamic = ["version"]
+
+[project.scripts]
+package-releases-job = "producer:main"
+
+[build-system]
+requires = ["setuptools", "wheel", "setuptools_scm"]
+
+[tool.setuptools_scm]


This might not be needed for this component, as this is not released as module, just as a container image on release.

sesheta · 2023-01-18T22:26:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign harshad16 for approval by writing /assign @harshad16 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

VannTen · 2023-01-20T10:22:50Z

On Wed, Jan 18, 2023 at 02:26:34PM -0800, Harshad Reddy Nalla wrote: @harshad16 requested changes on this pull request. On reviewing, its seems, i missed earlier that this component is not module. we don't need the project in pyproject.toml and can keep the version.py for this component. as it would be released as container image. > +[project] +name = "package-releases-job" +description = "Check packages updates on Python package indexes" +readme = "README.rst" +requires-python = ">=3.8" +license = {file = "LICENSE"} +dynamic = ["version"] + +[project.scripts] +package-releases-job = "producer:main" + +[build-system] +requires = ["setuptools", "wheel", "setuptools_scm"] + +[tool.setuptools_scm] This might not be needed for this component, as this is not released as module, just as a container image on release.

I don't disagree ; I still think we could get some benefits from converting to pyproject.toml, mainly standardizing all of thoth repos on a common model so we can treat them the same in the CI.

harshad16 · 2023-01-20T14:22:45Z

I don't disagree ; I still think we could get some benefits from converting to pyproject.toml, mainly standardizing all of thoth repos on a common model so we can treat them the same in the CI.

we are using pyproject.toml.
is there a PEP standard for having a version on pyproject.toml, it seems project which use poetry use that?
as this project is just a component, it feel version.py file is more easy and convenient.
WDYT ?

VannTen · 2023-01-20T14:33:43Z

Actually we can have both if having version defined inside a file is desirable setuptools support something like this in pyproject.toml [project] dynamic = ["version"] [tool.setuptools.dynamic] version = {attr: "version.version"} The main point is to avoid repeating the version twice and then forgetting to update it twice later on. Would that work ?

harshad16 · 2023-01-20T14:36:02Z

Actually we can have both if having version defined inside a file is desirable setuptools support something like this in pyproject.toml [project] dynamic = ["version"] [tool.setuptools.dynamic] version = {attr: "version.version"} The main point is to avoid repeating the version twice and then forgetting to update it twice later on. Would that work ?

yes, we can do this.
side question: what is the reason to have version in pyproject.toml in projects which are not python modules ?

VannTen · 2023-01-20T14:46:38Z

Well, handling them as python modules allows to handle them in a more general way. For example, using the repo as a pre-commit hook (like we're currently doing in adviser) rely on it being a python module. Some of the tooling (mypy in particular) seems to be a bit lost when it can't identify a module. Regarding scripts / job specifically, relying on [project.scripts] instead of manually invoking the python file guarantee we'll have the correct dependency in scope by just doing a `pip install .` of the repo. All of the above is my opinion but TL;DR: easier integration with the Python ecosystem and it's tooling.

harshad16 · 2023-01-20T14:53:30Z

Thank you for the explanation, i appreciate it.

For this PR, can we bring back the version.py and introduce the dynamic version in pyproject.toml

VannTen · 2023-01-20T15:01:49Z

Do you think we can have the `__version__` directly in `producer.py` ? Having one single file for that seems a bit strange. (Or maybe that's related to kebechet ?)

Use [project.script] instead of invoking python.

harshad16 · 2023-01-20T15:51:48Z

Do you think we can have the __version__ directly in producer.py ? Having one single file for that seems a bit strange. (Or maybe that's related to kebechet ?)

yeah that is related to kebechet
maybe can plan this future

sesheta requested review from harshad16 and KPostOffice September 19, 2022 09:11

sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2022

VannTen force-pushed the git_tags_version branch from bdb2a98 to 8e06110 Compare September 19, 2022 12:51

harshad16 reviewed Sep 20, 2022

View reviewed changes

VannTen mentioned this pull request Sep 21, 2022

[EPIC] Support for PEP 517 / PEP 566 thoth-station/kebechet#1144

Open

5 tasks

KPostOffice reviewed Dec 7, 2022

View reviewed changes

harshad16 requested changes Jan 18, 2023

View reviewed changes

VannTen added 2 commits January 20, 2023 16:21

Convert to single file python module

6962a40

Use [project.script] instead of invoking python.

Reoder imports

b554c37

VannTen force-pushed the git_tags_version branch from 8e06110 to b554c37 Compare January 20, 2023 15:23

sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2023

VannTen changed the title ~~Dynamic versionning (git tags)~~ Convert to python module (pyproject.toml) Jan 20, 2023

VannTen closed this Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to python module (pyproject.toml) #659

Convert to python module (pyproject.toml) #659

VannTen commented Sep 19, 2022

harshad16 left a comment

harshad16 Sep 20, 2022

VannTen Sep 21, 2022

harshad16 Sep 21, 2022

KPostOffice Dec 7, 2022

KPostOffice Dec 7, 2022

harshad16 left a comment

harshad16 Jan 18, 2023

sesheta commented Jan 18, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

Convert to python module (pyproject.toml) #659

Convert to python module (pyproject.toml) #659

Conversation

VannTen commented Sep 19, 2022

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 Sep 20, 2022

Choose a reason for hiding this comment

VannTen Sep 21, 2022

Choose a reason for hiding this comment

harshad16 Sep 21, 2022

Choose a reason for hiding this comment

KPostOffice Dec 7, 2022

Choose a reason for hiding this comment

KPostOffice Dec 7, 2022

Choose a reason for hiding this comment

harshad16 left a comment

Choose a reason for hiding this comment

harshad16 Jan 18, 2023

Choose a reason for hiding this comment

sesheta commented Jan 18, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023

VannTen commented Jan 20, 2023 via email

harshad16 commented Jan 20, 2023