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 the build portion of the gh-actions #249

Merged
merged 12 commits into from
Nov 25, 2024
Merged

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Nov 18, 2024

As a first review, this will:

  1. Fix the broken build action for cuda_bindings
  2. Begin the plumbing of python-version as a top level matrix parameter. Currently using 3.12 and the hardcoded cuda 12.6 from environment.yml. This means environment.yml is now autogenerated, otherwise it would consistently override whichever specified python with latest (3.13)
  3. Have the action run on both bindings and core

Copy link

copy-pr-bot bot commented Nov 18, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work ksimpson-work self-assigned this Nov 18, 2024
.github/actions/build/action.yml.j2 Outdated Show resolved Hide resolved
.github/workflows/ci-gh.yml Show resolved Hide resolved
continuous_integration/scripts/make-conda-env Show resolved Hide resolved
@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work ksimpson-work added P0 High priority - Must do! test Addition or improved tests and removed P0 High priority - Must do! labels Nov 19, 2024
@ksimpson-work
Copy link
Contributor Author

ksimpson-work commented Nov 19, 2024

@sandeepd-nv I cherry picked your commits to remove the dead code, and address the formatting change. Per the gh-runners slack channel, there is currently an issue which is causing actions nvidia-wide to hang. edit: runners are fixed

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

@rwgk rwgk mentioned this pull request Nov 20, 2024
@rwgk
Copy link
Collaborator

rwgk commented Nov 20, 2024

Wow the setup is pretty sophisticated. Looks good to me as a meaningful step forward, although I'm not familiar with the details. One high-level question, maybe for another PR: Could the build be organized so that building cuda.bindings and cuda.core appear as two separate steps in the build log? The idea is to make it very obvious in the log what step completed successfully, e.g.

> Build cuda.bindings
> Test cuda.bindings
> Build cuda.core
> Test cuda.core

Where the > are the clickable arrows in the log, for expanding or un-expanding each part of the log.

@ksimpson-work
Copy link
Contributor Author

ksimpson-work commented Nov 20, 2024

@rwgk yeah.. It took me awhile to wrap my head around the flow of logic. My plan was to make as few changes as possible just to get a build and test working. Then create some gh-issues to track improvements like splitting the bindings and core + builds and tests. It would be simple to do it naively, but to do it in an efficient way (minimizing container and conda spin-ups) would take some degree of thought/effort. I just want to have some automated testing ASAP. What are your thoughts on that approach?

ref: #249

@rwgk
Copy link
Collaborator

rwgk commented Nov 20, 2024

What are your thoughts on that approach?

Working incrementally sounds good.

I just looked at the latest "raw" build log here, below is a small section.

I wonder, what happens if you simply add echo '##[group]Building cuda.bindings' etc. from the bash script(s)? Does enable the expand/collapse functionality when viewing the log?

(I think the timestamps are automatic.)

2024-11-20T18:40:33.9334459Z ##[group]Initializing the repository
2024-11-20T18:40:33.9338653Z [command]/usr/bin/git init /home/runner/_work/cuda-python/cuda-python
2024-11-20T18:40:33.9709849Z hint: Using 'master' as the name for the initial branch. This default branch name
2024-11-20T18:40:33.9711359Z hint: is subject to change. To configure the initial branch name to use in all
2024-11-20T18:40:33.9712302Z hint: of your new repositories, which will suppress this warning, call:
2024-11-20T18:40:33.9712951Z hint:
2024-11-20T18:40:33.9713621Z hint: 	git config --global init.defaultBranch <name>
2024-11-20T18:40:33.9714055Z hint:
2024-11-20T18:40:33.9714619Z hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
2024-11-20T18:40:33.9715399Z hint: 'development'. The just-created branch can be renamed via this command:
2024-11-20T18:40:33.9715903Z hint:
2024-11-20T18:40:33.9716247Z hint: 	git branch -m <name>
2024-11-20T18:40:33.9716893Z Initialized empty Git repository in /home/runner/_work/cuda-python/cuda-python/.git/
2024-11-20T18:40:33.9722034Z [command]/usr/bin/git remote add origin https://github.com/NVIDIA/cuda-python
2024-11-20T18:40:33.9766281Z ##[endgroup]
2024-11-20T18:40:33.9767383Z ##[group]Disabling automatic garbage collection
2024-11-20T18:40:33.9772721Z [command]/usr/bin/git config --local gc.auto 0
2024-11-20T18:40:33.9806106Z ##[endgroup]
2024-11-20T18:40:33.9806753Z ##[group]Setting up auth
2024-11-20T18:40:33.9809801Z [command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
2024-11-20T18:40:33.9837453Z [command]/usr/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
2024-11-20T18:40:34.0603151Z [command]/usr/bin/git config --local --name-only --get-regexp http\.https\:\/\/github\.com\/\.extraheader
2024-11-20T18:40:34.0630441Z [command]/usr/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :"
2024-11-20T18:40:34.0845435Z [command]/usr/bin/git config --local http.https://github.com/.extraheader AUTHORIZATION: basic ***
2024-11-20T18:40:34.0882172Z ##[endgroup]

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

I will look into that. Sounds like it could be the perfect easy solution @rwgk

@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work ksimpson-work merged commit 0c426b0 into main Nov 25, 2024
1 check passed
@ksimpson-work ksimpson-work deleted the ksimpson/fix-gh-actions branch November 25, 2024 19:29
@leofang leofang added P0 High priority - Must do! CI/CD CI/CD infrastructure and removed test Addition or improved tests labels Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD CI/CD infrastructure P0 High priority - Must do!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants