Skip to content

Unify Common CI Code for Windows and Linux#645

Merged
leofang merged 4 commits into
NVIDIA:mainfrom
cryos:ci-unify-bash
May 29, 2025
Merged

Unify Common CI Code for Windows and Linux#645
leofang merged 4 commits into
NVIDIA:mainfrom
cryos:ci-unify-bash

Conversation

@cryos

@cryos cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator

Description

Unify CI code to share common code between different workflows, closes #603.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

copy-pr-bot Bot commented May 19, 2025

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

1 similar comment
@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

1 similar comment
@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@github-actions

This comment has been minimized.

@cryos

cryos commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos cryos added the CI/CD CI/CD infrastructure label May 19, 2025
@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

1 similar comment
@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@cryos

cryos commented May 20, 2025

Copy link
Copy Markdown
Collaborator Author

/ok to test

@leofang leofang added this to the cuda.core beta 4 milestone May 20, 2025
@leofang leofang added the P1 Medium priority - Should do label May 20, 2025
@cryos cryos changed the title Test out a simple env-vars script for build Unify Common CI Code for Windows and Linux May 20, 2025
@leofang leofang added the enhancement Any code-related improvements label May 21, 2025
@leofang

leofang commented May 21, 2025

Copy link
Copy Markdown
Member

LGTM so far. I see the assumption here is that the env vars are set in the CI and we just run the scripts, so the scripts are not meant to be run outside of the CI. This is how we avoid param checks (which would add a ton of boilerplates). Let me know whenever you're happy to merge!

@cryos

cryos commented May 21, 2025

Copy link
Copy Markdown
Collaborator Author

LGTM so far. I see the assumption here is that the env vars are set in the CI and we just run the scripts, so the scripts are not meant to be run outside of the CI. This is how we avoid param checks (which would add a ton of boilerplates). Let me know whenever you're happy to merge!

Thanks Leo! There are a few other bits I wanted to centralize first, I was proving out that things would work as expected on Windows first. I can split that and merge this if preferred, then create follow ups later to avoid it growing too big.

@leofang

leofang commented May 21, 2025

Copy link
Copy Markdown
Member

Up to you -- so far I've been able to follow just fine 🙂

Comment thread ci/tools/env-vars
# We don't test compute-sanitizer on CTK<12 because backporting fixes is too much effort
# We only test compute-sanitizer on python 3.12 arbitrarily; we don't need to use sanitizer on the entire matrix
# Only local ctk installs have compute-sanitizer; there is no wheel for it
if [[ "${PY_VER}" == "3.12" && "${CUDA_VER}" != "11.8.0" && "${LOCAL_CTK}" == 1 && "${HOST_PLATFORM}" == linux* ]]; then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, good catch, this will fix #647.

@leofang leofang linked an issue May 23, 2025 that may be closed by this pull request
@cryos cryos marked this pull request as ready for review May 23, 2025 14:54
@copy-pr-bot

copy-pr-bot Bot commented May 23, 2025

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@cryos cryos requested a review from leofang May 23, 2025 14:55

@leofang leofang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Comment thread ci/tools/env-vars
SETUP_SANITIZER=1
else
SETUP_SANITIZER=0
echo "SANITIZER_CMD=" >> $GITHUB_ENV

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: no need to set it at this stage?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You still want this for workflows that do not call setup-sanitizer as the variable would be unbound. It is nice not to force all workflows to call setup-sanitizer. We can remove it if we have all workflows call the setup, which thinking about it we could just do from the start of run-tests as it relies on the sanitizer stuff being ready there.

@leofang leofang merged commit d1c8b0f into NVIDIA:main May 29, 2025
56 checks passed
@github-actions

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

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

Labels

CI/CD CI/CD infrastructure enhancement Any code-related improvements P1 Medium priority - Should do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: compute-sanitizer package is not correctly cached CI: Explore consolidating Linux/Windows test workflows?

2 participants