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

Use self-hosted azure runners for bionemo unit tests #560

Closed

Conversation

pstjohn
Copy link
Collaborator

@pstjohn pstjohn commented Dec 31, 2024

No description provided.

@pstjohn pstjohn force-pushed the pstjohn/self-hosted-runner-action branch from 0c864e0 to af85a9d Compare December 31, 2024 19:30
@pstjohn pstjohn force-pushed the pstjohn/self-hosted-runner-action branch 4 times, most recently from b910cfa to d2d08f4 Compare January 2, 2025 22:29
@pstjohn pstjohn force-pushed the pstjohn/self-hosted-runner-action branch from 4ac20e0 to 818f4dc Compare January 3, 2025 15:27
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@pstjohn pstjohn force-pushed the pstjohn/self-hosted-runner-action branch 2 times, most recently from c4c2284 to 12e09d6 Compare January 5, 2025 15:26
@pstjohn
Copy link
Collaborator Author

pstjohn commented Jan 5, 2025

Looks like test-results-action isn't working correctly, since this PR is coming from a fork rather than an internal branch and doesn't have access to repository secrets.
So I think the options are

  1. Only run test analytics on main branch runs
  2. figure out tokenless test analytics uploads
  3. use on: pull_request_target and make sure we understand the security implications there

@pstjohn pstjohn force-pushed the pstjohn/self-hosted-runner-action branch 2 times, most recently from 781a8f2 to df87ab1 Compare January 8, 2025 23:48
@pstjohn pstjohn changed the title initial attempt to use self-hosted azure runners for bionemo unit tests Use self-hosted azure runners for bionemo unit tests Jan 8, 2025
@pstjohn pstjohn force-pushed the pstjohn/self-hosted-runner-action branch from df87ab1 to 7633699 Compare January 9, 2025 01:46
Comment on lines -39 to -45
# Function to handle cleanup on script exit
cleanup() {
local exit_code=$?
[ -n "${coverage_files[*]:-}" ] && rm -f "${coverage_files[@]:-}"
exit "$exit_code"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need these coverage files to persist to the next step in the GHA actions pipeline so we can upload them to codecov. Do these stick around on the blossom workers?

path: ${{ github.run_id }}

- name: Run tests
run: ./ci/scripts/run_pytest.sh --no-nbval
Copy link
Collaborator Author

@pstjohn pstjohn Jan 9, 2025

Choose a reason for hiding this comment

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

we could eventually make this configurable based on the PR label 🤷 . But I wonder how that interacts with code coverage diffs. I.e., I wouldn't want to flag a PR as "decreasing coverage" because we didn't run notebook tests...

Copy link
Collaborator

@dorotat-nv dorotat-nv left a comment

Choose a reason for hiding this comment

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

Reviewed, and added ask to add mechanism for PR labels, otherwise LGTM

path: ${{ github.run_id }}

- name: Run tests
run: ./ci/scripts/run_pytest.sh --no-nbval
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add here mechanism for PR labels: SKIP_CI and INCLUDE_NOTEBOOKS_TESTS to define the scope of tests or omit them completely?

@pstjohn
Copy link
Collaborator Author

pstjohn commented Jan 9, 2025

Testing from a branch rather than a fork in #587

@pstjohn pstjohn closed this Jan 9, 2025
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

Successfully merging this pull request may close these issues.

4 participants