Use hatch-vcs for dynamic version numbering from git

[jacobtomlinson]

Closes #102

[jacobtomlinson]

@jameslamb things are failing here because I remove the VERSION file, but I'm not 100% sure where it's being used and how to safely remove it. Would you mind taking a look?

[jameslamb]

Sure. So the conda builds are failing like this:

head: cannot open './VERSION' for reading: No such file or directory

(build link)

The immediate cause of that is this line, which expects that file to exist:

rapids-cli/ci/build_python.sh

Lines 10 to 11 in 6e8d487

	RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION)
	export RAPIDS_PACKAGE_VERSION

Notice there that the environment variable RAPIDS_PACKAGE_VERSION is getting set... that's for re-use here:

rapids-cli/conda/recipes/rapids-cli/recipe.yaml

Line 5 in 6e8d487

version: ${{ env.get("RAPIDS_PACKAGE_VERSION") }}

rapids-cli/conda/recipes/rapids-cli/recipe.yaml

Line 14 in 6e8d487

version: ${{ version }}

Ultimately, that package.version field in the rattler-build recipe needs to get set. I don't know how to do that with something like hatch-vcs or setuptools_scm, you will have to try to figure that part out. The recipes on conda-forge using hatch-vcs may be a good guide: https://github.com/search?q=org%3Aconda-forge%20%22hatch-vcs%22&type=code

[jacobtomlinson]

@jameslamb I both switched to a conda package install and am also using hatchling directly instead of hatch which keeps the dependencies low. It's a shame conda recipes don't support looking up the version dynamically at build time.

[jameslamb]

I checked the logs to understand what this would mean for versioning. If this PR is merged, conda packages and wheels with a version number similar to the following would be published to the nightly conda channel and PyPI index.

rapids-cli-0.1.3.dev19+g5db20cc

Writing this out for my own understanding:

• 0.1.3 = assume a patch bump over the version from the latest tag (v0.1.2)
• .dev19 = 20 commits since the v0.1.2 tag (11 on main, 9 more on this PR's branch)
• +g5db20cc = literal g followed by the first 7 characters of the commit SHA of the latest commit
  • 5db20cc
  • https://github.com/ofek/hatch-vcs/blob/dfec2c06defdb4648eb66f6aadf7b57453d732cd/hatch_vcs/metadata_hook.py#L39

Seems fine to me.

With this tool though, how do we ensure that all that .dev{n}+g{sha} stuff is omitted when building release artifacts? e.g., ensure that tag v0.1.3 triggers a build of exactly 0.1.3?

---

I left some more suggestions and questions, but marking this "approve" so you can do what you want with them and merge without needed a re-review from me. @ me if you want another review.

[jameslamb]

Suggested change

	rapids-mamba-retry install \
	--yes \
	'hatchling' 'hatch-vcs'
	rapids-mamba-retry install \
	--yes \
	'hatchling>=1.27.0' \
	'hatch-vcs>=0.5.0'

Could we please add floors (to help speed up this conda solve) and put one package on each line?

Fine with me to leave these unpinned in the conda recipe / pyproject.toml to support a wider range of build environments, but here in CI for this separate CLI invocation we don't need that amount of flexibility... might as well add the floors to try to save a little CI time (via quicker conda solves).

Putting floors on build dependencies is also nice to help reduce the risk of hard-to-debug issues of the form "something changed in the environment and instead of outright failing conda made all the constraints work by choosing an ancient version of some packages".

[jameslamb]

What's the purposes of this try-except?

If it's just to convince mypy that attributes like __version__ exist, when it can't tell that from the files in source control because _version.py is auto-generated, I think it should be removed, in favor of some mix of the following:

• adding an __all__ = ["__version__", "__version_tuple__"] in this module
• using type: ignore comments in any place mypy complains about

This try-except makes me nervous because it could silently fail... you might not catch in CI that we've published a package with __version__ = "0.0.0" accidentally.

So if you do keep it, I'd at least ask that you please add unit tests similar to this:

from rapids_cli import __version__, __version_tuple__

def versioning_minimally_worked():
  assert __version__ != "0.0.0"
  assert __version_tuple__ != (0, 0, 0)

[jacobtomlinson]

The _version.py file only gets created during install or sdist/wheel build so it's just there so that if you clone the repo and then run pytest without doing pip install -e . things actually work.

[jameslamb]

if you clone the repo and then run pytest without doing pip install -e . things actually work

Is pip install -e . for this small, pure-python project so expensive that avoiding it is worth taking on a slight added risk of shipping a package with a silently-broken __version__ variable?

I personally don't think so, but this is a minor matter of personal preference and much more your project than mine, so I'll defer to your judgment here.

[jacobtomlinson]

I think it was more that the first time I ran into this it was a hard to debug gotcha. Maybe instead of catching the ImportError and setting the values it would be better to reraise the exception with a more helpful error message.

[jacobtomlinson]

With this tool though, how do we ensure that all that .dev{n}+g{sha} stuff is omitted when building release artifacts? e.g., ensure that tag v0.1.3 triggers a build of exactly 0.1.3?

If you are on the tag with a clean git status then the version will resolve exactly to the tag. The ending is only added if there are additional commits or an unclean git status. This is common functionality across tools like versioneer/setuptools-scm/hatch-vcs.

[jameslamb]

The ending is only added if there are additional commits or an unclean git status

Ok thanks, "unclean git status" being part of this is surprising to me, but all seems fine. Just wanted to be sure we'd considered how this would work for releases.

[jameslamb]

Removing my blocking review, thanks for answering my questions.