Migrate from versioneer to setuptools_scm #1375

maresb · 2022-12-17T22:06:58Z

The base of this PR is #1371. The first original commit on this branch is "Remove versioneer".

In the first commit I remove versioneer and set the version to "TODO". In the second commit, I add setuptools_scm.

I also attempt to correctly configure the nightly builds. I do this by running aesara.misc.prepare_nightly_build to modify pyproject.toml in the nightly build, updating the following entries:

[project]
name = "aesara-nightly"
dynamic = []
version = "[generated from aesara.misc.prepare_nightly_build:nightly_version_number]"

[tool.setuptools_scm]
(deleted)

This should be tested, but I'm reasonably confident that it will work properly, and I raise an error if the version number has an incorrect form.

One strange quirk which I don't quite understand is that in order to type-check aesara.misc.prepare_nightly_build I added the types-toml package to the mypy pre-commit config, and afterwards it raised an error about an unused type: ignore in aesara/scan/op.py L3430. Removing the type: ignore resolved the error. 🤷‍♂️

Thank you for opening a PR!

Here are a few important guidelines and requirements to check before your PR can be merged:

There is an informative high-level description of the changes.
The description and/or commit message(s) references the relevant GitHub issue(s).
pre-commit is installed and set up.
The commit messages follow these guidelines.
The commits correspond to relevant logical changes, and there are no commits that fix changes introduced by other commits in the same branch/BR.
There are tests covering the changes introduced in the PR.

Don't worry, your PR doesn't need to be in perfect order to submit it. As development progresses and/or reviewers request changes, you can always rewrite the history of your feature/PR branches.

If your PR is an ongoing effort and you would like to involve us in the process, simply make it a draft PR.

codecov · 2022-12-18T21:10:44Z

Codecov Report

Merging #1375 (9dae630) into main (96eca11) will increase coverage by 0.10%.
The diff coverage is n/a.

❗ Current head 9dae630 differs from pull request most recent head 9d46e0f. Consider uploading reports for the commit 9d46e0f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1375      +/-   ##
==========================================
+ Coverage   74.59%   74.69%   +0.10%     
==========================================
  Files         196      194       -2     
  Lines       49813    49730      -83     
  Branches    10547    10527      -20     
==========================================
- Hits        37157    37145      -12     
+ Misses      10331    10262      -69     
+ Partials     2325     2323       -2

Impacted Files	Coverage Δ
aesara/bin/aesara_cache.py

brandonwillard

In general, this looks good; just a few comments/corrections.

Aside from that, what other approaches are there for the nightly build process? It seems generating a custom pyproject.toml could increase our maintenance overhead.

brandonwillard · 2022-12-20T00:48:57Z

aesara/scan/op.py

-@op_debug_information.register(Scan)  # type: ignore[has-type]
+@op_debug_information.register(Scan)


This is fine if we can somehow guarantee that we won't start seeing #1221 again; otherwise, we don't want to reintroduce it.

Oh yikes, that looks tricky! Thanks for the context. I will need to follow up here to try and understand better what's going on.

Yeah, I would really like to fix that problem at the source, but it was/is annoyingly elusive.

Working around this by adding warn_unused_ignores = false as below and reverting the removal of #type: ignore, but I'll still take a stab at understanding what's really going on.

Unfortunately I haven't gotten a chance to look into this yet. The issue no longer creeps up here since readding warn_unused_ignores = false in #1376. Is this blocking the merge?

aesara/version.py

pyproject.toml

brandonwillard · 2022-12-20T00:58:52Z

readthedocs.yml

+  jobs:
+    post_checkout:
+      # This is necessary for setuptools_scm to properly read the tags. The default
+      # depth of 50 often leads to 'assert version is not None' AssertionError during
+      # the pip install build process. Alternatively, we could use
+      # 'git fetch --unshallow', but that is rather intensive.
+      - git fetch --depth 1000


I need to understand more about this, because it seems like it could be indicative of a general issue with this approach.

My understanding is that we compute the version by looking through the git history to find the latest matching git tag. The default commands (see the RTD CI logs) download a truncated history. This command runs afterwards to download more. I chose 1000 because it's way more than I think we would ever need, and way smaller than the whole commit history. What Ida at don't understand is why it was no prob w versioneer

Are you still concerned about this? I think --depth 1000 is better than --unshallow (i.e. depth=\infty) due to bandwidth/CI time considerations, and 1000 is way more than we could conceivably need.

No, that's not a real concern.

I have a vague suspicion about what might be going on here...

In case I'm working from a fork, then it's possible that my fork might not have all the tags. (Git is sometimes a bit weird about synchronizing tags, although mainly in the push direction.) Then setuptools_scm may not have the info to compute the version.

Typing this out, it doesn't make much sense, but at some point during testing I somehow ended up installing a very "old" version of aesara simply because the tags were old. I don't have any actionable ideas, this is just something to think about.

maresb · 2022-12-20T16:16:50Z

@brandonwillard, how would you feel about first merging #1371? Otherwise this PR has lots of big commits.

Thanks for the review! Looks great at first glance. Will go through it in detail now...

brandonwillard · 2022-12-20T16:46:16Z

@brandonwillard, how would you feel about first merging #1371? Otherwise this PR has lots of big commits.

Yeah, I'll look at that next.

maresb · 2022-12-25T20:05:59Z

TODO: see if any of the pre-defined setuptools-scm formats are pushable to PyPI. If so, eliminate the nightly script.

maresb · 2022-12-26T11:25:15Z

Details about the construction of version numbers are as follows. But this may be superfluous given the suggestion proposed in the next comment...

There are two parts to a setuptools_scm version number: the "version scheme" followed by the "local scheme".

These are both defined in terms of entry points, so the options are obtainable with

from setuptools_scm import get_version
from importlib.metadata import entry_points

version_schemes = [
    ep.name for ep in entry_points(group="setuptools_scm.version_scheme")
]
local_schemes = [
    ep.name for ep in entry_points(group="setuptools_scm.local_scheme")
]
print(f"{version_schemes=}\n\nlocal_schemes={local_schemes}")

version_schemes=['calver-by-date', 'guess-next-dev', 'no-guess-dev', 'post-release', 'python-simplified-semver', 'release-branch-semver']

local_schemes=['dirty-tag', 'no-local-version', 'node-and-date', 'node-and-timestamp']

To first inspect just the version_scheme, we can set local_scheme="no-local-version":

print(
    {
        version_scheme: get_version(
            version_scheme=version_scheme, local_scheme="no-local-version"
        )
        for version_scheme in version_schemes
    }
)

{'calver-by-date': '22.12.25.0.dev97', 'guess-next-dev': '2.8.10.dev97', 'no-guess-dev': '2.8.9.post1.dev97', 'post-release': '2.8.9.post97', 'python-simplified-semver': '2.8.10.dev97', 'release-branch-semver': '2.9.0.dev97'}

Note how all of the possibilities except for calver-by-date, no-guess-dev, and post-release increment the version number in some way.

The most concise possibility is post-release, so next we fix this and look at local_scheme, which depends on whether or not the repository is clean:

print("Clean:")
print(
    {
        local_scheme: get_version(
            version_scheme="post-release", local_scheme=local_scheme
        )
        for local_scheme in local_schemes
    }
)
!echo >> pyproject.toml
print("Dirty:")
print(
    {
        local_scheme: get_version(
            version_scheme="post-release", local_scheme=local_scheme
        )
        for local_scheme in local_schemes
    }
)
!git checkout -q pyproject.toml

Clean:
{'dirty-tag': '2.8.9.post97', 'no-local-version': '2.8.9.post97', 'node-and-date': '2.8.9.post97+g1945ad8cb', 'node-and-timestamp': '2.8.9.post97+g1945ad8cb'}
Dirty:
{'dirty-tag': '2.8.9.post97+dirty', 'no-local-version': '2.8.9.post97', 'node-and-date': '2.8.9.post97+g1945ad8cb.d20221226', 'node-and-timestamp': '2.8.9.post97+g1945ad8cb.d20221226110450'}

maresb · 2022-12-26T11:32:40Z

Why not just do version_scheme="post-release", local_scheme="no-local-version". Then the version number will look like 2.8.9.post97, where 97 is the number of commits on main since the last release. (git checkout @~97 switches to rel-2.8.9.) Rather than running the job nightly, we can run it on merge-to-main, which is probably more useful anyways.

Getting the version number set correctly could then be accomplished in a GH Actions workflow with a few simple dasel commands to set version_scheme and local_scheme in pyproject.toml, and would eliminate the need for Python scripts.

Then aesara-nightly is no longer a fitting name, so either we leave it as-is for "historical reasons" or we come up with something else.

maresb · 2022-12-26T15:40:56Z

Just pushed a quick implementation of the idea of my last post (only tested locally).

maresb · 2022-12-29T21:50:55Z

Rebased on #1376

maresb · 2022-12-30T12:16:27Z

Rebased on main

brandonwillard · 2022-12-30T17:53:11Z

.github/workflows/dev-builds.yml

+          curl -sSLf https://github.com/TomWright/dasel/releases/download/v2.0.2/dasel_linux_amd64 \
+            -L -o /tmp/dasel && chmod +x /tmp/dasel
+
+          # Modify pyproject.toml to set the nightly version in the form of
+          # x.y.z.postN, where N is the number of commits since the last release
+          /tmp/dasel put -f pyproject.toml project.name -v aesara-nightly
+          /tmp/dasel put -f pyproject.toml tool.setuptools_scm.version_scheme -v post-release
+          /tmp/dasel put -f pyproject.toml tool.setuptools_scm.local_scheme -v no-local-version


This looks neat!

Great, then let's keep this in favor of the Python script-based approach. (Will squash the commit which introduces this.)

Squashed. What do you think about the dev builds being called aesara-nightly while being built on merge-to-main?

That sounds good.

brandonwillard

Looks good. As always, it's hard to tell when/if these kinds of changes will work right away, but we can always follow up!

brandonwillard · 2022-12-31T21:05:13Z

.github/workflows/dev-builds.yml

+          curl -sSLf https://github.com/TomWright/dasel/releases/download/v2.0.2/dasel_linux_amd64 \
+            -L -o /tmp/dasel && chmod +x /tmp/dasel
+
+          # Modify pyproject.toml to set the nightly version in the form of
+          # x.y.z.postN, where N is the number of commits since the last release
+          /tmp/dasel put -f pyproject.toml project.name -v aesara-nightly
+          /tmp/dasel put -f pyproject.toml tool.setuptools_scm.version_scheme -v post-release
+          /tmp/dasel put -f pyproject.toml tool.setuptools_scm.local_scheme -v no-local-version


That sounds good.

brandonwillard · 2022-12-31T21:06:18Z

readthedocs.yml

+  jobs:
+    post_checkout:
+      # This is necessary for setuptools_scm to properly read the tags. The default
+      # depth of 50 often leads to 'assert version is not None' AssertionError during
+      # the pip install build process. Alternatively, we could use
+      # 'git fetch --unshallow', but that is rather intensive.
+      - git fetch --depth 1000


No, that's not a real concern.

maresb · 2023-01-01T23:37:35Z

This requires at least one more iteration, since setuptools_scm completely replaces the selection of files in the package (Git-driven). See ea65078 for all the changes. In particular,

I noticed that aesara/scan/scan_perform.pyx is missing from the current distributions. It looks important, so I added it.
I noticed that two files were being included under tests/link/c/c_code by virtue of having the .c and .h extensions. This looks wrong, so I removed it.

Additionally, I noticed that aesara-cache is being installed in a top-level module called bin. This seems very undesirable, as it should probably instead go under aesara.bin. I will make a subsequent PR to address this.

maresb · 2023-01-03T21:39:25Z

Just rebased on main. All checks were green.

rlouf · 2023-01-06T12:31:42Z

One question: won't this cause issues when running the tests locally if the user hasn't built the package locally since there won't be any _version.py file?

maresb · 2023-01-06T15:25:17Z

The _version.py file is generated not only by building but also by installing. So as long as the user runs pip install [--editable] . as they should, it should work.

What can however go wrong is that for an --editable install, if the version number changes, then you have to rerun pip install -e . in order to trigger an update to _version.py. Also if you have a clone which is somehow missing some release tags, then the version may also be incorrect. (That happened to me at some point as I described in #1375 (comment), but unfortunately I don't remember exactly how.)

maresb · 2023-01-06T22:08:58Z

I'm not sure what to think of this:

Warning: Performance alert! Previous value was 61258.103085955845 and current value is 28070.87678331834. It is 2.182265397650837x worse than previous exceeding a ratio threshold 2
Warning: Performance alert! Previous value was 1.3921410587885548 and current value is 0.6737363168561912. It is 2.066299565510444x worse than previous exceeding a ratio threshold 2
Error: # ⚠️ Performance Alert ⚠️

maresb · 2023-01-06T22:11:37Z

Hmm, dependabot did bump the version on the performance tests. I wonder if that has anything to do with it.

On the other hand, the benchmarks succeeded in #1384. Maybe this simply deployed to slow runners.

brandonwillard · 2023-01-07T00:13:32Z

Hmm, dependabot did bump the version on the performance tests. I wonder if that has anything to do with it.

On the other hand, the benchmarks succeeded in #1384. Maybe this simply deployed to slow runners.

Looks like it's one of the JAX benchmarks, so it could be due to a number of things (e.g. compilation, test seeding, etc.) I'll put in a PR to at least guarantee that the test data is the same every time.

maresb · 2023-01-09T14:16:53Z

The benchmarks failed again.

I wonder if the automerge failed to properly await the coverage results and prematurely merged #1382 (comment).

Since I believe I know what's wrong, I prepended a commit to this PR to hopefully fix it. @brandonwillard, do you still approve of a rebase (no squash) merge with the extra commit?

maresb · 2023-01-09T14:19:09Z

Is it possible to rerun the "Benchmarks" test without rerunning the whole thing?

Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.noreply.github.com>

Avoids 'assert version is not None' from setuptools_scm during the pip install build process

maresb · 2023-01-10T02:05:25Z

@brandonwillard, tests were all green before rebase. Are we okay to automerge (rebase) now, given the extra commit I added for fixing coverage?

maresb · 2023-01-10T03:28:10Z

@brandonwillard thanks, now I'm just missing an approving review. (It wouldn't allow me to self-approve this time.)

maresb · 2023-02-05T08:53:25Z

Addendum

This PR unintentionally dropped support for git archive. See #1405 for a fix.

brandonwillard added testing CI Involves continuous integration and removed testing labels Dec 18, 2022

maresb force-pushed the use-setuptools-scm branch from 55d953a to 365daae Compare December 18, 2022 19:35

maresb marked this pull request as ready for review December 18, 2022 23:42

brandonwillard reviewed Dec 20, 2022

View reviewed changes

maresb force-pushed the use-setuptools-scm branch 2 times, most recently from 2879403 to 1945ad8 Compare December 25, 2022 20:03

maresb force-pushed the use-setuptools-scm branch 2 times, most recently from a1758db to 8ed07cd Compare December 29, 2022 21:50

maresb force-pushed the use-setuptools-scm branch from 8ed07cd to 9dae630 Compare December 30, 2022 12:15

brandonwillard reviewed Dec 30, 2022

View reviewed changes

maresb force-pushed the use-setuptools-scm branch 2 times, most recently from cfa9802 to 1c860d6 Compare December 30, 2022 19:20

maresb mentioned this pull request Dec 31, 2022

Failing nightly build #1379

Closed

brandonwillard previously approved these changes Dec 31, 2022

View reviewed changes

maresb dismissed brandonwillard’s stale review via 691fe99 December 31, 2022 21:22

maresb force-pushed the use-setuptools-scm branch from 1c860d6 to 691fe99 Compare December 31, 2022 21:22

brandonwillard previously approved these changes Jan 1, 2023

View reviewed changes

maresb dismissed brandonwillard’s stale review via ea65078 January 1, 2023 23:36

maresb mentioned this pull request Jan 2, 2023

Move aesara-cache into aesara.bin package #1382

Merged

6 tasks

maresb force-pushed the use-setuptools-scm branch from a15e3e1 to 2e16eb4 Compare January 3, 2023 21:39

maresb force-pushed the use-setuptools-scm branch from 2e16eb4 to a792d7a Compare January 6, 2023 21:47

maresb force-pushed the use-setuptools-scm branch from a792d7a to 3bddb24 Compare January 7, 2023 08:15

brandonwillard force-pushed the use-setuptools-scm branch from 3bddb24 to cf93bb6 Compare January 9, 2023 05:33

brandonwillard previously approved these changes Jan 9, 2023

View reviewed changes

maresb dismissed brandonwillard’s stale review via e6af3d6 January 9, 2023 08:54

maresb force-pushed the use-setuptools-scm branch 2 times, most recently from e6af3d6 to 55d0a4d Compare January 9, 2023 13:30

maresb and others added 4 commits January 10, 2023 03:04

Move aesara_cache.py omits to coverage.run

4ce25ea

Remove versioneer

5f60814

Compute version with setuptools_scm

5b567e2

Co-authored-by: Brandon T. Willard <971601+brandonwillard@users.noreply.github.com>

Increase Git fetch depth for readthedocs

9d46e0f

Avoids 'assert version is not None' from setuptools_scm during the pip install build process

maresb force-pushed the use-setuptools-scm branch from 55d0a4d to 9d46e0f Compare January 10, 2023 02:04

brandonwillard enabled auto-merge (rebase) January 10, 2023 02:06

brandonwillard approved these changes Jan 10, 2023

View reviewed changes

brandonwillard merged commit e71bd78 into aesara-devs:main Jan 10, 2023

maresb deleted the use-setuptools-scm branch January 10, 2023 03:30

brandonwillard added the enhancement New feature or request label Jan 10, 2023

		@op_debug_information.register(Scan) # type: ignore[has-type]
		@op_debug_information.register(Scan)

Migrate from versioneer to setuptools_scm #1375

Migrate from versioneer to setuptools_scm #1375

Conversation

maresb commented Dec 17, 2022 • edited Loading

codecov bot commented Dec 18, 2022 • edited Loading

Codecov Report

brandonwillard left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb commented Dec 20, 2022

brandonwillard commented Dec 20, 2022

maresb commented Dec 25, 2022

maresb commented Dec 26, 2022

maresb commented Dec 26, 2022 • edited Loading

maresb commented Dec 26, 2022

maresb commented Dec 29, 2022

maresb commented Dec 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonwillard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb commented Jan 1, 2023

maresb commented Jan 3, 2023

rlouf commented Jan 6, 2023 • edited Loading

maresb commented Jan 6, 2023

maresb commented Jan 6, 2023 • edited Loading

maresb commented Jan 6, 2023 • edited Loading

brandonwillard commented Jan 7, 2023

maresb commented Jan 9, 2023

maresb commented Jan 9, 2023

maresb commented Jan 10, 2023 • edited Loading

maresb commented Jan 10, 2023

maresb commented Feb 5, 2023

Addendum

maresb commented Dec 17, 2022 •

edited

Loading

codecov bot commented Dec 18, 2022 •

edited

Loading

brandonwillard left a comment •

edited

Loading

maresb Dec 20, 2022 •

edited

Loading

maresb commented Dec 26, 2022 •

edited

Loading

rlouf commented Jan 6, 2023 •

edited

Loading

maresb commented Jan 6, 2023 •

edited

Loading

maresb commented Jan 6, 2023 •

edited

Loading

maresb commented Jan 10, 2023 •

edited

Loading