Tests for evaluation with NVIDIA Evals Factory
[!IMPORTANT]
TheUpdate branchbutton must only be pressed in very rare occassions. An outdated branch is never blocking the merge of a PR. Please reach out to the automation team before pressing that button.
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Collection: [Note which collection this PR will affect]
Changelog
- Add specific line by line info of high level changes in this PR.
Usage
- You can potentially add a usage example below
# Add a code snippet demonstrating how to use this
GitHub Actions CI
The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.
The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR. To re-run CI remove and add the label again. To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".
Before your PR is "Ready for review"
Pre checks:
- [ ] Make sure you read and followed Contributor guidelines
- [ ] Did you write any new necessary tests?
- [ ] Did you add or update any necessary documentation?
- [ ] Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
- [ ] Reviewer: Does the PR have correct import guards for all optional libraries?
PR Type:
- [ ] New Feature
- [ ] Bugfix
- [ ] Documentation
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed. Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information
- Related to # (issue)
@marta-sd Unfortunately, there's some merge conflicts now that need to be resolved to get the CI to run. Please let me know if you'd like some help.
CI is good, shall we merge?
@ko3n1g unfortunately not - Optional_L2_NeMo_2_EVAL was added in this PR and it's failing, I need to fix it first. Also I added tests to cicd-main-export-deploy which was skipped. Is there a way to trigger them?
@ko3n1g unfortunately not -
Optional_L2_NeMo_2_EVALwas added in this PR and it's failing, I need to fix it first. Also I added tests tocicd-main-export-deploywhich was skipped. Is there a way to trigger them?
Oh I see, thanks! Could you remove the is-optional: true marker in that case? Also, could you remove the Optional_ prefix from its name? We don’t need this anymore nowadays.
Skipping export-deploy should be fixed by a change we rolled out last night, I’ll keep an eye on the CI
@chtruong814 @ko3n1g curious why we have moved the eval unit tests to .github/workflows/cicd-main-export-deploy.yml ? Thanks!
@chtruong814 @ko3n1g curious why we have moved the eval unit tests to
.github/workflows/cicd-main-export-deploy.yml? Thanks!
We were under the impression that they are closely related, especially from dependency perspective. Where would you rather see those?
@chtruong814 @ko3n1g curious why we have moved the eval unit tests to
.github/workflows/cicd-main-export-deploy.yml? Thanks!We were under the impression that they are closely related, especially from dependency perspective. Where would you rather see those?
I am okay for it to stay in .github/workflows/cicd-main-export-deploy.yml unless .github/workflows/cicd-main-export-deploy.yml would be moving to a separate repo (nemo-export-deploy repo) shortly. The eval code would continue to live in collections/llm, so these tests are needed here.
@chtruong814 @ko3n1g curious why we have moved the eval unit tests to
.github/workflows/cicd-main-export-deploy.yml? Thanks!We were under the impression that they are closely related, especially from dependency perspective. Where would you rather see those?
I am okay for it to stay in
.github/workflows/cicd-main-export-deploy.ymlunless.github/workflows/cicd-main-export-deploy.ymlwould be moving to a separate repo (nemo-export-deploy repo) shortly. The eval code would continue to live in collections/llm, so these tests are needed here.
Ok gotcha. Let's move the tests to nemo2 with a follow-up PR. We had to act under little time when we split the monolithic CI into smaller pieces, so this was more a human error than on purpose. Thanks for pointing this out!
CI has passed, feel free to merge
I had to resolve conflicts but I think we're good to go now
Closing (changes merged in #13627 )