Skip to content

.GITLAB: add UCX pkg-config to env - #518

Closed
roiedanino wants to merge 1 commit into
ai-dynamo:mainfrom
roiedanino:python-ci
Closed

roiedanino wants to merge 1 commit into
ai-dynamo:mainfrom
roiedanino:python-ci

Conversation

@roiedanino

Copy link
Copy Markdown
Contributor

What?

Add UCX pkg-config to path in test_python.sh

Why?

Otherwise meson can't find UCX when building and running python tests

Signed-off-by: Roie Danino <rdanino@nvidia.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds the UCX pkg-config directory to the environment in the test script to ensure Meson can find the UCX installation when building and running Python tests.

  • Introduces a new argument (UCX_INSTALL_DIR) to the script.
  • Updates the usage message and appends the UCX pkg-config directory to PKG_CONFIG_PATH.
Comments suppressed due to low confidence (1)

.gitlab/test_python.sh:22

  • [nitpick] Consider adding an inline comment to clarify that UCX_INSTALL_DIR represents the installation directory of UCX, improving readability for future maintainers.
UCX_INSTALL_DIR=$2

@github-actions

github-actions Bot commented Jul 1, 2025

Copy link
Copy Markdown

👋 Hi roiedanino! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

@roiedanino

Copy link
Copy Markdown
Contributor Author

/build

Comment thread .gitlab/test_python.sh

if [ -z "$INSTALL_DIR" ]; then
echo "Usage: $0 <install_dir>"
if [ -z "$INSTALL_DIR" ] || [ -z "$UCX_INSTALL_DIR" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default behavior today is to put both Nixl and UCX in same install dir which is how all build/tests scripts works today.
Your change forces us to provide both places even when it same place, i would have it as optional or you need to change all the places that calls this script (gitlab, aws, blossom etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants