Skip to content

Dynamic versioning for postrelease versions#465

Merged
evagroenendijk merged 13 commits intomasterfrom
dont_overwrite_symlinks
Jun 4, 2025
Merged

Dynamic versioning for postrelease versions#465
evagroenendijk merged 13 commits intomasterfrom
dont_overwrite_symlinks

Conversation

@evagroenendijk
Copy link
Contributor

@evagroenendijk evagroenendijk commented May 20, 2025

I adjusted the typo you mentioned & adjusted the metadata.py s.t. it can read an eko made by its own version. I tested it and it now creates eko's with the postrelease addition. It would be great if you could check it @felixhekhorn !

@felixhekhorn felixhekhorn added the bug Something isn't working label May 20, 2025
@felixhekhorn felixhekhorn linked an issue May 20, 2025 that may be closed by this pull request
Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

First, some stupid remarks:

  • For our own records it's better to have a more descriptive title of a PR. "Solve issue 464" is not very clear afterwards, since you need to know what 464 was in the first place. GitHub allows to link PR and issues in a native way, so we can make that connection elsewhere (I did it here already) (see also here)
  • similar considerations apply to commit messages: let's try to keep them more descriptive and to the point - I know it is not easy. If you haven't done so yet, please read this blogpost 🙃

Second, more serious comments:

  • can you please add an explicit unit test against this? Instead of me checking it once, let's automatise. So, it either has to be a definite version XOR 0.0.0-postNUMBER-COMMIT ; i.e. in tests/eko/io/test_struct.py
  • can you please update the CHANGELOG? modify the existing entry with the correct format and just append this PR

@evagroenendijk
Copy link
Contributor Author

First, some stupid remarks:

  • For our own records it's better to have a more descriptive title of a PR. "Solve issue 464" is not very clear afterwards, since you need to know what 464 was in the first place. GitHub allows to link PR and issues in a native way, so we can make that connection elsewhere (I did it here already) (see also here)
  • similar considerations apply to commit messages: let's try to keep them more descriptive and to the point - I know it is not easy. If you haven't done so yet, please read this blogpost 🙃

Second, more serious comments:

  • can you please add an explicit unit test against this? Instead of me checking it once, let's automatise. So, it either has to be a definite version XOR 0.0.0-postNUMBER-COMMIT ; i.e. in tests/eko/io/test_struct.py
  • can you please update the CHANGELOG? modify the existing entry with the correct format and just append this PR

Thanks a lot for your comments, will do! Then I wanted to ask one other thing, if you have time: in the metadata.py I have now added this extra condition wrt the version, but was wondering if there is a more elegant way to define the eko version that you are using? I remember that defining stuff with strings was not so nice, but I also did not really find a direct command for it

@evagroenendijk evagroenendijk changed the title Solve issue #464 Dynamic versioning for postrelease versions May 20, 2025
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
evagroenendijk and others added 3 commits May 22, 2025 10:10
@felixhekhorn
Copy link
Contributor

felixhekhorn commented May 22, 2025

I think the remaining problem is that poetry-dynamic-versioning is not working correctly.
From here you see it wants to do the right thing, but this is then not correctly written to the source code. Please check how to tell it where to write the information to (=src/eko/version.py). Now that I have written that sentence and have been thinking about it: what about the other modules, meaning ekobox + ekomark? they should have the same version and we need to tell them

PS: from here you see still the hardcoded version is present (instead of the dynamic one)

@evagroenendijk
Copy link
Contributor Author

evagroenendijk commented May 29, 2025

I think the remaining problem is that poetry-dynamic-versioning is not working correctly. From here you see it wants to do the right thing, but this is then not correctly written to the source code. Please check how to tell it where to write the information to (=src/eko/version.py). Now that I have written that sentence and have been thinking about it: what about the other modules, meaning ekobox + ekomark? they should have the same version and we need to tell them

PS: from here you see still the hardcoded version is present (instead of the dynamic one)

Sorry for my late reply, I was a bit busy with other things! What I don't completely understand is that when I run the unit tests on my computer (with installed eko version 0.0.0.post57+374c7f9b) everything does go correctly. Also if I make an eko with the lha benchmark, in the metadata it shows the correct version of(0.0.0-post.57+374c7f9b). So I don't really get why here it is not working correctly

NB regarding ekobox and ekomark, do you want to also define some version.py file in the src there?

@felixhekhorn
Copy link
Contributor

NB regarding ekobox and ekomark, do you want to also define some version.py file in the src there?

Let me quickly try to ping @alecandido and ask for his opinion: We have one package (eko), but several modules (eko, ekomark, ekobox) - each of the modules should define it's own version (via __version__) and "by chance" they are always the same, do you agree? In principle (if only we had infinite time), we could e.g. declare ekobox our public frontend and decouple that

@alecandido
Copy link
Member

Ok, the situation is slightly more complicated. But in essence you're right @felixhekhorn.

Just to get the correct terminology, let me recap Python's conceptual units:

  • a module is whatever unit you can import - and it is mapped one-to-one on individual files; i.e. the module is a Python class itself, and an instance is a memory representation of a unit container of objects, which is (usually) loaded from a file (it could be created dynamically - but forget about this)
  • a Python package is a collection of modules and further packages, which are then called subpackages (the usual recursive tree type, as for folders) - what we often call just "package" is a top-level one
  • a distribution/PyPI/pip package is a collection of files (mostly Python packages - possibly multiple ones - and metadata), which is installed via a package manager

So, the only one that has to be a version is the distribution package eko. This version is the one registered on PyPI and used by package managers for installation.
However, it is customary to expose a __version__ attribute (purely conventional) as a top-level Python package attribute. Everyone would expect this value to agree with the one known to the package manager (which instead is stored in the metadata, not in the source).

Thus, having a single distribution package, the multiple __version__ are actually expected to be the same.
Currently, only the eko package is declaring it, and only that one is updated by the dynamic versioning

eko/pyproject.toml

Lines 98 to 107 in 67140bc

[tool.poetry-dynamic-versioning.substitution]
files = ["src/eko/version.py"]
folders = [{path = "src/eko"}]
[tool.poetry-dynamic-versioning.files."src/eko/version.py"]
persistent-substitution = true
initial-content = """
__version__ = "0.0.0"
__data_version__ = 3
"""

Notice that the motivation to have multiple packages is just because of a least-worst attempt at dependency management, and a poor-man solution for a monorepo.
All the code is always installed and available, but the idea is that, if you import a package other than the main one, you can expect you may need extra dependencies.
The proper solution would have been to have fully independent packages which are separately distributed, each with his own version and __version__ attribute (which could also be independent, since they would have independent public interfaces). But it would result in a much more involved development environment setup and management.

@felixhekhorn
Copy link
Contributor

regarding ekobox and ekomark, do you want to also define some version.py file in the src there?

the answer is: yes, please (they don't need there own __data_version__)

@evagroenendijk evagroenendijk merged commit c08fc01 into master Jun 4, 2025
9 checks passed
@felixhekhorn felixhekhorn deleted the dont_overwrite_symlinks branch December 3, 2025 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Version format key is not correct

3 participants