-
Notifications
You must be signed in to change notification settings - Fork 107
Merge update version and fix version format bugs #286
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
Merge update version and fix version format bugs #286
Conversation
|
@jameslamb may I get your review on this |
jameslamb
left a comment
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.
Now that cuopt and RAPIDS will have aligned versions, there is a lot of opportunity for consolidating things in this new update-version.sh script. I left some specific examples I saw where things should be grouped into a smaller set of updates.
That'll make it run a little faster, reduce the risk of inconsistencies, and reduce the effort to maintain this.
Please also test this. Something like this:
./ci/release/update-version.sh '25.12.00'
# does the diff look like what you'd expect?
git diff
# did it catch everything?
git grep -E '24\.|25\.'|
wanted to keep them separate in case if we need to be out of sync in future, but I think it can be done quite easily, will update PR as per your review |
jameslamb
left a comment
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 for the updates. I've left some more suggestions.
|
@jameslamb May I get your review on this ? |
jameslamb
left a comment
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.
Approving this so you can keep moving forward. But if you haven't already, please do test it one more time, in the way I described in #286 (review)
I also think you should write up a follow-up issue about consolidating the references to versions in this project. I left a couple of suggestions, but overall there are very very many hard-coded 25.10 around the source code that all need to be kept in sync.
For inspiration, you could look at some of the PRs @KyleFromNVIDIA put up around RAPIDS implementing similar things to what I've suggested here: rapidsai/build-planning#15
| # Server version update | ||
| sed_runner 's/'"\"version\": \"[0-9][0-9].[0-9][0-9]\""'/'"\"version\": \"${NEXT_SHORT_TAG}\""'/g' python/cuopt_server/cuopt_server/utils/data_definition.py | ||
| sed_runner 's/'"\"client_version\": \"[0-9][0-9].[0-9][0-9]\""'/'"\"client_version\": \"${NEXT_SHORT_TAG}\""'/g' python/cuopt_server/cuopt_server/utils/routing/data_definition.py | ||
| sed_runner 's/'"\"client_version\": \"[0-9][0-9].[0-9][0-9]\""'/'"\"client_version\": \"${NEXT_SHORT_TAG}\""'/g' python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py |
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 think it would be good to calculate this programmatically, in Python code instead.
e.g.
| "client_version": "25.10", |
Should rely on this constant:
| __version__ = ( |
Same advice goes for all 3 of these lines.
| sed_runner 's/'"\"client_version\": \"[0-9][0-9].[0-9][0-9]\""'/'"\"client_version\": \"${NEXT_SHORT_TAG}\""'/g' python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py | ||
|
|
||
| # Doc update | ||
| sed_runner 's/'"version = \"[0-9][0-9].[0-9][0-9]\""'/'"version = \"${NEXT_SHORT_TAG}\""'/g' docs/cuopt/source/conf.py |
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.
This should use cuopt.__version__ instead.
Example of how cudf does this: https://github.com/rapidsai/cudf/blob/21b69cf8c31c42bf981c5c8987b4d0d6ff36b3d5/docs/cudf/source/conf.py#L189-L195
|
|
||
| # Doc update | ||
| sed_runner 's/'"version = \"[0-9][0-9].[0-9][0-9]\""'/'"version = \"${NEXT_SHORT_TAG}\""'/g' docs/cuopt/source/conf.py | ||
| sed_runner 's/'"PROJECT_NUMBER = [0-9][0-9].[0-9][0-9]"'/'"PROJECT_NUMBER = ${NEXT_SHORT_TAG}"'/g' cpp/doxygen/Doxyfile |
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.
Doxyfiles can be templated and filled in at runtime with environment variables. This project should consider doing that.
Example from raft:
| # Update VERSIONS.json | ||
| VERSIONS_FILE="docs/cuopt/source/versions1.json" | ||
| # Only update if NEXT_FULL_TAG is not already present | ||
| if ! grep -q "\"version\": \"${NEXT_FULL_TAG}\"" "${VERSIONS_FILE}"; then | ||
| # Remove preferred and latest flags, but keep the version entry and URL | ||
| sed_runner '/"name": "latest",/d' "${VERSIONS_FILE}" | ||
| sed_runner '/"preferred": true,\?/d' "${VERSIONS_FILE}" | ||
| # Remove trailing comma after "url": ... in all version entries | ||
| sed_runner 's/\("url": "[^"]*"\),/\1/' "${VERSIONS_FILE}" | ||
| # Add new version entry with both preferred and latest flags | ||
| NEW_VERSION_ENTRY=' {\n "version": "'${NEXT_FULL_TAG}'",\n "url": "../'${NEXT_FULL_TAG}'/",\n "name": "latest",\n "preferred": true\n },' | ||
| sed_runner "/\[/a\\${NEW_VERSION_ENTRY}" "${VERSIONS_FILE}" | ||
| fi |
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 think you might find this easier to maintain if it was in Python.
I recommend looking into what I just did over in the docs repo:
- post-processing: fix version map, use pre-commit to regenerate it rapidsai/docs#669
- post-processing: re-write generate-projects-to-versions in Python rapidsai/docs#670
I think you should have a small Python script that populates versions1.json, and a pre-commit hook that runs it.
And that script should read from ./VERSION at the root of the repo.
That'd simplify update-version.sh, and make it easier to keep that file up to date.
| # RTD update | ||
| sed_runner "/^set(cuopt_version/ s/[0-9][0-9].[0-9][0-9].[0-9][0-9]/${NEXT_FULL_TAG}/g" python/cuopt/CMakeLists.txt | ||
| sed_runner "/^set(cuopt_version/ s/[0-9][0-9].[0-9][0-9].[0-9][0-9]/${NEXT_FULL_TAG}/g" python/cuopt/cuopt/linear_programming/CMakeLists.txt | ||
| sed_runner "/^set(cuopt_version/ s/[0-9][0-9].[0-9][0-9].[0-9][0-9]/${NEXT_FULL_TAG}/g" python/libcuopt/CMakeLists.txt |
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.
It should be possible to derive all of these in CMake, at configure time, from other variables.
Re-using all of these variables:
cuopt/cmake/rapids_config.cmake
Lines 15 to 21 in b567ed4
| if(_rapids_version MATCHES [[^([0-9][0-9])\.([0-9][0-9])\.([0-9][0-9])]]) | |
| set(RAPIDS_VERSION_MAJOR "${CMAKE_MATCH_1}") | |
| set(RAPIDS_VERSION_MINOR "${CMAKE_MATCH_2}") | |
| set(RAPIDS_VERSION_PATCH "${CMAKE_MATCH_3}") | |
| set(RAPIDS_VERSION_MAJOR_MINOR "${RAPIDS_VERSION_MAJOR}.${RAPIDS_VERSION_MINOR}") | |
| set(RAPIDS_VERSION "${RAPIDS_VERSION_MAJOR}.${RAPIDS_VERSION_MINOR}.${RAPIDS_VERSION_PATCH}") | |
| else() |
| done | ||
|
|
||
| # PYTHON for RAPIDS | ||
| sed_runner "/DOWNLOAD.*rapids-cmake/ s/branch-[0-9][0-9].[0-9][0-9]/branch-${NEXT_SHORT_TAG}/g" python/cuopt/CMakeLists.txt |
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.
This should be grouped with the other CMakeLists.txt updates further up.
And similar to my comment on those, I think it'd be better to move this logic into CMake code itself.
Yes, I had tested by applying new version, but will test again once I have added new suggestions, thank you for such a detailed review. |
|
/merge |
Description
Merge update version which was earlier split was cuOpt and Rapids into one since we are in sync with versions now.
And also fix failure in version updates and bugs around it.
Issue
closes #262
Checklist