Repository navigation
Conversation
6cf455a to
a2fe261
Compare
e52499b to
144de2f
Compare
a582b10 to
2721563
Compare
2721563 to
0a1e855
Compare
akashchi
left a comment
There was a problem hiding this comment.
I have serious reservations about the overall direction. My core concern is that this represents a global over-complication of the CI that will further discourage developers from adding tests themselves, since it introduces a level of obscurity for the CI setup.
We deliberately moved away from this kind of complexity when we migrated the tests out of Jenkins into openvino GHA CI.
My main takeaways are:
- Re-implementing native GHA stuff handling in Python scripts
E.g., the PR moves artifact handling into custom Python scripts (e.g. the pull & build logic in module_openvino_plugin.yml) and container orchestration into .github/scripts/common/container_job.py. Why? GitHub Actions already provides this. I trust actions/download-artifact far more than a non-native (from a GitHub Actions perspective) approach to downloading artifacts. We have already implemented that in the Smart CI action. GitHub admittedly doesn't handle artifacts (download/upload) perfectly on its own, but the answer to that shouldn't be to route everything through hand-written Python.
- The
README.mdclaims "native"
The README states at the very top:
The CI uses native reusable workflows instead of a configuration DSL
...and is then followed by a large amount of custom Python code that is arguably closer to a DSL than what we have today. We already have native .yml workflows.
- Re-inventing existing GitHub Actions
Several hand-written utilities re-implement functionality that already exists as off-the-shelf GitHub Actions, e.g., detect_git_changes. We should prefer maintained, community actions over bespoke scripts we now have to own and maintain ourselves.
- Magic strings
There are a lot of magic strings scattered across the workflows, e.g., contrib-ci-build. These are fragile, undocumented coupling points that are easy to break and hard to trace.
- Scalability problem
module_openvino_plugin.yml, which acts as the entry point, already has 18 inputs while serving only a single plugin. For comparison, our job_build_linux.yml in the openvino repository has 20 inputs but covers the whole Linux build. An 18-input adapter per module does not scale well and undermines the "short module-owned adapter" description in the PR summary.
- Further adoption
If this pattern lands fully in openvino_contrib, the implicit next step is porting it to other repositories.
- Debugging
A key benefit of native .yml workflows is that a contributor can read the workflow and understand exactly what runs w/o delving into custom scripts. Of course, there are actions and reusable workflows, but the general flow, imo, should be defined in a .yml.
0a1e855 to
fcae24a
Compare
1a3dd2c to
6cae740
Compare
6cae740 to
8ca4327
Compare
|
@akashchi please review it again |
| persist-credentials: false | ||
|
|
||
| - name: Get prebuilt OpenVINO | ||
| uses: ./openvino_actions/.github/actions/restore_artifacts |
| "${NVIDIA_ARTIFACT}/bin/cuda-sanitizer.sh" | ||
|
|
||
| container=( | ||
| docker run --rm |
There was a problem hiding this comment.
Could you elaborate - why is it done like that? There's an option to schedule the entire GHA job within a container via "container" parameter, there should also be a flexibility for parameters setting.
| build: | ||
| name: Build | ||
| needs: images | ||
| runs-on: aks-linux-16-cores-32gb |
There was a problem hiding this comment.
Probably we could consider smaller and cheaper VM, if the build is not demanding for resources. @ababushk, what do you think?
| - name: Restore sccache | ||
| id: sccache | ||
| continue-on-error: true | ||
| uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 | ||
| with: | ||
| path: ${{ env.SCCACHE_DIR }} | ||
| key: nvidia-ubuntu22-release-${{ github.ref_name }}-${{ github.sha }} | ||
| restore-keys: | | ||
| nvidia-ubuntu22-release-${{ github.ref_name }}- | ||
| nvidia-ubuntu22-release-master- |
There was a problem hiding this comment.
Why use a GHA cache? It is small and slower compared to a mounted one.
Our self-hosted runners have access to a mounted share with sccache cache, see https://github.com/openvinotoolkit/openvino/blob/2ff7a717c624d2b460e72bfdfdecd974d8391dba/.github/workflows/job_build_linux.yml#L138
https://github.com/openvinotoolkit/openvino/blob/2ff7a717c624d2b460e72bfdfdecd974d8391dba/.github/workflows/job_build_linux.yml#L146
https://github.com/openvinotoolkit/openvino/blob/2ff7a717c624d2b460e72bfdfdecd974d8391dba/.github/workflows/job_build_linux.yml#L116
only need to mount and specify a unique key.
|
This pull request has been automatically marked as stale because it has not had any activity for the last 2 weeks. It will be closed in 7 days if no further activity occurs. Please push new commits or leave a comment to keep it open. Thank you for your contributions! |
|
This pull request has been automatically closed because it has been stale for 7 days with no activity. Feel free to reopen it if you would like to continue the work. Thank you for your contributions! |
Summary
Introduces a small module-oriented CI entry point for
openvino_contrib, withnvidia_pluginas the first independent module workflow.Architecture
overall_status.ymlis the repository orchestrator. It runs the existing OpenVINO Smart CI implementation to select affected modules, calls each module workflow as an equal peer, and aggregates their results inci/gha_overall_status..github/components.ymlcontains only Smart CI dependency relationships. Unknown modules are validated conservatively until their own workflow is registered.module_<name>.ymlowns the complete readable flow for that module: images, runners, containers, build commands, artifacts, hardware and tests. There is no generic lifecycle with a growing input list and no custom CI DSL.NVIDIA migration
sccache.module_nvidia_plugin.yml; the only Python helper validates the module-generated plugin registry and collects the minimal runtime for transfer.test_cuda.yml,sanitizer_cuda.yml, andhistory_cuda.ymlworkflows.Safety
no-new-privileges, a PID limit, bounded temporary storage, and only the NVIDIA devices exposed.