Skip to content
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

Fix CI installed version to not include commit offset #5

Open
seberg opened this issue Nov 13, 2024 · 1 comment
Open

Fix CI installed version to not include commit offset #5

seberg opened this issue Nov 13, 2024 · 1 comment

Comments

@seberg
Copy link
Contributor

seberg commented Nov 13, 2024

I thought that changing:

- LEGATEDATAFRAME_VERSION=$(rapids-version)
+ LEGATEDATAFRAME_VERSION="$(head -1 ./VERSION)"

in the docs build and test scripts when installing was a good idea to make things strict, but it seems like it is not actually safe (as noted by @jameslamb).

So, I should change it back (possibly also in one place in legate-dataframe).

A bit unclear if we need to build the .dev<N> version at all in that case, but maybe it is nice to just do it anyway.

@seberg seberg transferred this issue from another repository Nov 22, 2024
@jameslamb
Copy link
Member

Suspect this was copied from an old legate-dataframe issue? Here in legate-raft we never set the variable LEGATEDATAFRAME_VERSION in CI scripts.

I think it's referring to this:

rapids-generate-version > ./VERSION
LEGATERAFT_VERSION="$(head -1 ./VERSION)"

rapids-generate-version > ./VERSION
LEGATERAFT_VERSION="$(head -1 ./VERSION)"

Some notes:

  1. Using rapids-generate-version does not necessarily mean "including commit offsets"... for release builds (triggered by a tag matching the pattern ^v[0-9][0-9].[0-9][0-9].[0-9]$), the version number computed by that command will not have commit distance, e.g. it'd be 24.11.00, not something like 24.11.00.dev17
  2. Yes, using rapids-generate-version in those scripts is unsafe. For non-release builds, it computes a version containing the tag distance (number of commits since last git tag) at runtime. That could return a different answer between package-build time and docs-build or testing time, e.g. if other PRs are merged in between those runs. We should be using doing what legate-boost does... using rapids-generate-version to update the VERSION file, but then always grepping out of it just the {major}.{minor}.{patch} part.
rapids-generate-version > ./VERSION
LEGATEBOOST_VERSION=$(rapids-version)

(legate-boost/ci/test_python_common.sh)

That has its own risks ... it could silently fall back to something else published on the legate conda channel instead of the downloaded packages built in the same CI run. That can be avoided with conda's "strict channel priority"... something that isn't guaranteed to work (yet) for dependency trees involving the rapidsai / rapidsai-nighly channels, but which hopefully will soon: rapidsai/build-planning#84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants