-
Notifications
You must be signed in to change notification settings - Fork 94
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
Remove versioneer #1204
Remove versioneer #1204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Peter.
@@ -3,7 +3,6 @@ build-backend = "setuptools.build_meta" | |||
requires = [ | |||
"setuptools>=64.0.0", | |||
"tomli ; python_version < '3.11'", | |||
"versioneer>=0.24", | |||
] | |||
|
|||
[project] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since version
was removed from setup.py
, I believe it needs to be added here instead (eg https://github.com/rapidsai/cudf/blob/branch-23.08/python/dask_cudf/pyproject.toml#L12). And I think the dynamic
field should be removed.
Also, an entry needs to be added to update-version.sh
to update this file's version too.
You can see that no version is being used in the wheel build: https://github.com/rapidsai/dask-cuda/actions/runs/5377443612/jobs/9755962220?pr=1204#step:4:54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we need to ensure that nightlies have the a${RAPIDS_DATE_STRING}
appended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @raydouglass for catching that. I've done the changes in 210937e and it looks like the build version is correct now: https://github.com/rapidsai/dask-cuda/actions/runs/5379525987/jobs/9760908314#step:4:54 .
We have RAPIDS_DATE_STRING
being passed to the package version in meta.yaml
, which seems consistent with cuDF. Is there anywhere else we need to pass it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since we upload nightlies to pypi.org, they need a "pre-release" version. This would be something like 23.8.0a230626
(here).
I'm not exactly sure the best way to implement this. We bypass this issue in other rapids libraries by uploading them to our own nightly index and they use a unix epoch in the version to distinguish, but they are not pre-releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is acceptable, but I've attempted something in 937919b . That change will essentially change version
in pyproject.toml
before building the wheel and later revert it. I don't know how we would identify whether we are doing a release build (and thus not modify pyproject.toml
) or a pre-release build that we want to do that change, any ideas?
Obviously, we would also need to update ci/release/update-version.sh
to do the necessary changes in this file if we go ahead with that solution, but want to check first whether this is how we want to move ahead.
cc @vyasr who may have insights here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding how you can decide whether or not to make the change, inside this script you have access to a bash function that you can use: if rapids-is-release-build; do ...; done
.
I think the current solution is acceptable. In the long run I would like to move the rest of RAPIDS to proper pre-releases, but that will require additional changes to our dependency management so I hadn't considered the practical question of how we would do it. The workflows for other RAPIDS packages are just different enough that the exact approach would probably end up being slightly different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, you guys thought of everything! 😄
I think everything is correct now, including wheel version. Would you mind taking a look @vyasr @raydouglass ? |
ci/build_python_pypi.sh
Outdated
@@ -8,11 +8,36 @@ python -m pip install build --user | |||
export GIT_DESCRIBE_TAG=$(git describe --abbrev=0 --tags) | |||
export GIT_DESCRIBE_NUMBER=$(git rev-list ${GIT_DESCRIBE_TAG}..HEAD --count) | |||
|
|||
# Build date for PyPI pre-releases | |||
export RAPIDS_VERSION_NUMBER="23.08" | |||
export RAPIDS_FULL_VERSION_NUMBER="${RAPIDS_VERSION_NUMBER}.00" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't hardcode the patch version (unless you also update via update-version.sh
but we do not do this in any other repo).
Since during a release, update-version.sh
is run before building, you could pull the current version from pyproject.toml
. Then only adjust it to the pre-release version for not release builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version is being updated in https://github.com/rapidsai/dask-cuda/pull/1204/files#diff-8a0188e09c831b0e6ca8c1c51ab640efd1e519b1d7f94ab6251c9f0b90a82304R51 . I have no preference for getting the version from pyproject.toml
or updating it with update-version.sh
(except the latter is already done, and perhaps slightly easier). Do you have a strong preference for the pyproject.toml
option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a weak pref for pyproject.toml, not a strong one. FWIW here's how we do it for our other wheels: https://github.com/rapidsai/gha-tools/blob/main/tools/rapids-pip-wheel-version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small things.
ci/build_python_pypi.sh
Outdated
@@ -8,11 +8,36 @@ python -m pip install build --user | |||
export GIT_DESCRIBE_TAG=$(git describe --abbrev=0 --tags) | |||
export GIT_DESCRIBE_NUMBER=$(git rev-list ${GIT_DESCRIBE_TAG}..HEAD --count) | |||
|
|||
# Build date for PyPI pre-releases | |||
export RAPIDS_VERSION_NUMBER="23.08" | |||
export RAPIDS_FULL_VERSION_NUMBER="${RAPIDS_VERSION_NUMBER}.00" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a weak pref for pyproject.toml, not a strong one. FWIW here's how we do it for our other wheels: https://github.com/rapidsai/gha-tools/blob/main/tools/rapids-pip-wheel-version
ci/build_python_pypi.sh
Outdated
# Inplace sed replace; workaround for Linux and Mac | ||
function sed_runner() { | ||
sed -i.bak ''"$1"'' $2 && rm -f ${2}.bak | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid proliferating the sed_runner
function. It has problems, and we don't need it here. This script is only ever run in CI on Linux machines. Let's just use plain sed for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in d4d887a .
Done in 4117cf4 . @raydouglass @vyasr would you mind taking another look? |
Thanks @raydouglass @vyasr @wence- ! |
/merge |
Proposes removing this project's build-time dependency on `tomli`. It appears to no longer be necessary. ```shell git grep tomli ``` ## Notes for Reviewers I originally noticed something similar in `ucx-py` (rapidsai/ucx-py#1042), then went searching for similar cases across RAPIDS. I'm not sure why this project has a dependency on `tomli`, but I suspect it was related to the use of `versioneer` in this project's history. Reference: python-versioneer/python-versioneer#338 (comment) This project doesn't use `versioneer` any more (#1204). I strongly suspect that the dependency on `tomli` can be removed. Authors: - James Lamb (https://github.com/jameslamb) Approvers: - https://github.com/jakirkham - Ray Douglass (https://github.com/raydouglass) URL: #1338
Closes #1203