Conversation
@microsoft-github-policy-service agree [company="microsoft"] |
44faaca to
ec09cba
Compare
|
@microsoft-github-policy-service agree |
2523a23 to
0ebed45
Compare
d73bb8e to
82fb7b9
Compare
fe5a82e to
78f787f
Compare
f22f8ef to
d94a2d6
Compare
- Add GitHub Actions workflow using official devcontainers/ci@v0.3 action - Add Azure DevOps pipeline with custom templates for macOS compatibility - Configure pytest with coverage reporting and JUnit XML output - Handle workspace permission issues using /tmp with sudo - Support CPU, GPU, and notebooks project configurations - Add coverage merging and test result publishing for both platforms - Skip pre-commit hook installation in CI when .git not available - Add sync-to-azdo workflow for pipeline synchronization - Update documentation with CI/CD setup instructions GitHub Actions uses the official action which works well on Linux. Azure DevOps uses custom templates to handle macOS-specific Docker issues (path translation and working directory mount namespace errors).
672b9c6 to
164e6b9
Compare
a279838 to
5f6316e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0e665e8 to
5f6316e
Compare
|
|
||
| # Speckit / .specify related files | ||
| .specify/ | ||
| specs/ | ||
| *.speckit | ||
|
|
||
| # .github agents and prompts | ||
| .github/agents/ | ||
| .github/prompts/ |
There was a problem hiding this comment.
should these be out of .gitignore? we often manage .github/agents/ .github/prompts/ as a team and share prompts
| # For users who need AML, create .env in your project folder (e.g., src/sample_pytorch_gpu_project/.env) | ||
| # Optional: Set these variables if using the AML CLI v2 example under src/sample_pytorch_gpu_project |
| "containerEnv": { | ||
| "PYTHONPATH": "/workspaces/repo" |
There was a problem hiding this comment.
Is this a wrong path? Also as long as .env is there, it should be picked up automatically by vscode so this one is not needed
| "postCreateCommand": "pre-commit install --overwrite", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", | ||
| "remoteUser": "devuser", |
There was a problem hiding this comment.
USER $USERNAME in Dockerfile side applies the same effect so this would be redundant
| }, | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", | ||
| "remoteUser": "devuser", |
There was a problem hiding this comment.
this should be automatically set by "ghcr.io/devcontainers/features/common-utils:2" above?
| } | ||
| } | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
is this your vscode's setting? it'd be good to keep diff minimal as this PR is quite large and format change may not be worth including?
| } | ||
| }, | ||
| } | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
is this your vscode's setting? it'd be good to keep diff minimal as this PR is quite large and format change may not be worth including?
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", | ||
| "remoteUser": "devuser", | ||
| "postCreateCommand": "cd /workspaces/repo && pre-commit install --overwrite || echo 'pre-commit installation failed; continuing without pre-commit hooks'", |
There was a problem hiding this comment.
is there any benefit of still going ahead when pre-commit fails? I'd rather it fail there as something would be wrong in that case
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", | ||
| "remoteUser": "devuser", | ||
| "postCreateCommand": "cd /workspaces/repo && pre-commit install --overwrite || echo 'pre-commit installation failed; continuing without pre-commit hooks'", |
| "postCreateCommand": "pre-commit install --overwrite", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", | ||
| "remoteUser": "devuser", |
There was a problem hiding this comment.
USER $USERNAME in Dockerfile side applies the same effect so this would be redundant
| ], | ||
| "postCreateCommand": "pre-commit install --overwrite", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", |
There was a problem hiding this comment.
any strong reason we want to introduce this path and realted settings? Otherwise, I'd prefer to remove it to keep devcontainer.json bare minimum
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", | ||
| "remoteUser": "devuser", | ||
| "postCreateCommand": "cd /workspaces/repo && pre-commit install --overwrite || echo 'pre-commit installation failed; continuing without pre-commit hooks'", |
| ], | ||
| "postCreateCommand": "pre-commit install --overwrite", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", |
There was a problem hiding this comment.
any strong reason we want to introduce this path? Otherwise, I'd prefer to remove it to keep devcontainer.json bare minimum
| "${localWorkspaceFolder}/.env" | ||
| ], | ||
| "postCreateCommand": "pre-commit install --overwrite", | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", |
There was a problem hiding this comment.
if we don't introduce "/workspaces/repo, I think this mount automatically runs without us specifying anything
| "username": "devuser" | ||
| } | ||
| }, | ||
| "initializeCommand": "test -f ${localWorkspaceFolder}/.env || touch ${localWorkspaceFolder}/.env", |
There was a problem hiding this comment.
how about creating .env from .env.example instead?
| "dockerfile": "Dockerfile" | ||
| }, | ||
| "shutdownAction": "none", | ||
| "updateRemoteUserUID": false, |
There was a problem hiding this comment.
default setting =true allows devcontainer to match local side UID and avoids file permission issues from happening (ex: files created within container owned by different UID and local can't access it). Is there any benefit with this? I see file access issue being avoided would be better but I'm curious to hear your thoughts
| "hostRequirements": { | ||
| "gpu": "optional" | ||
| }, |
There was a problem hiding this comment.
I didn't know about this option! This is cool as it can still work when gpu is not available while supporting gpu!
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/repo,type=bind", | ||
| "workspaceFolder": "/workspaces/repo", |
| # Azure DevOps pipeline for CI (Microsoft-hosted version) | ||
| # As the Microsoft-hosted agent option has a limit of 10GB of storage for disk outputs from a pipeline, | ||
| # this causes an issue when the Docker images for modules under src require more than 10GB of storage. | ||
| # If you will run into space issues (or other limitations with a Microsoft hosted agent option outlined in | ||
| # https://learn.microsoft.com/en-us/azure/devops/pipelines/agents/hosted?view=azure-devops&tabs=yaml#capabilities-and-limitations), | ||
| # consider using the .azuredevops/ado-ci-pipeline-self-hosted.yml version or using scale set agents, see | ||
| # this link for more info: https://learn.microsoft.com/en-us/azure/devops/pipelines/agents/scale-set-agents?view=azure-devops | ||
| # Note that docker images will only be build for src directories that contain at least one test file, so the | ||
| # total space consumed by Docker builds will be dependent on which modules under src contain tests. | ||
| # For setting up the pipeline in ADO see: | ||
| # https://learn.microsoft.com/en-us/azure/devops/pipelines/agents/pools-queues?view=azure-devops&tabs=yaml%2Cbrowser |
There was a problem hiding this comment.
what's the reason for diff showing up here? It looks the same?
| pool: | ||
| vmImage: 'ubuntu-latest' |
There was a problem hiding this comment.
I see every stage uses the same pool. so should we stick with the previous design where we define it once for all?
| # but could be adapated to other OSs. | ||
|
|
||
| # Azure DevOps CI Pipeline (Self-Hosted Agent) | ||
| # |
There was a problem hiding this comment.
this 10GB part was the reason why we needed two different yamls. Could you check if it's ok to delete this part? If we still have this restriction, we likely need some cleanup method we had for self-hosted version specifically
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
| # Run linter on codebase | ||
| lint: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Python 3.11 | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| name: test-and-coverage-results | ||
| path: | | ||
| **/test-reuslts-*.xml | ||
| coverage.xml | ||
| python-version: 3.11 | ||
|
|
||
| - name: Install requirements | ||
| run: | | ||
| python -m venv venv | ||
| source venv/bin/activate | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements-dev.txt | ||
|
|
||
| - name: Run ruff linter | ||
| run: | | ||
| source venv/bin/activate | ||
| ruff check --output-format github |
There was a problem hiding this comment.
It'd be better to keep this at the previous position as this part would run faster and if we fail at linter, we don't need to proceed further and that'd help us iterate faster
| filename: 'coverage/**/*.xml' | ||
|
|
||
| - name: Add code coverage summary markdown to github step summary | ||
| - name: Add code coverage summary to job summary |
| # Smoke test for notebooks devcontainer | ||
| test-notebooks: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup .env file | ||
| run: cp .env.example .env | ||
|
|
||
| - name: Build and smoke test notebooks devcontainer | ||
| uses: devcontainers/ci@v0.3 | ||
| with: | ||
| configFile: notebooks/.devcontainer/devcontainer.json | ||
| push: never | ||
| runCmd: | | ||
| cd /workspaces/repo | ||
| python --version | ||
| pytest --version | ||
| ruff --version |
There was a problem hiding this comment.
could we merge this to the previous section's build check?
| @@ -13,44 +13,109 @@ permissions: | |||
| contents: read | |||
There was a problem hiding this comment.
I wonder if it's really worth splitting into several jobs instead of having having them as multiple steps based on job complextity we have. One thing I noticed is this does repo clone 4 times as it used to be just one for example and that's one of downsides of going with this structure
fujikosu
left a comment
There was a problem hiding this comment.
Thanks for your PR! I left several comments so please check them
This pull request introduces a major refactor and modernization of the CI pipeline infrastructure for both Azure DevOps and GitHub Actions. The main improvements are the switch to modular, template-driven pipeline definitions, enhanced support for devcontainer-based testing, and improved code coverage reporting. The changes streamline CI setup, make it easier to add new projects, and improve compatibility across platforms (Linux and macOS). Documentation and configuration files have also been updated for clarity and consistency.
Azure DevOps Pipeline Modernization
.azuredevops/ado-ci-pipeline-ms-hosted.ymland.azuredevops/ado-ci-pipeline-self-hosted.ymlare refactored to use a two-stage pipeline (Lint and Test), with jobs and steps defined via new templates for devcontainer testing and coverage publishing. This replaces legacy scripts and direct task definitions, making the pipeline more maintainable and extensible. [1] [2] [3]Template-Driven CI Architecture
.azuredevops/templates/, includingtest-devcontainer-job.ymlfor per-project devcontainer testing,run-devcontainer.ymlfor platform-aware container execution,setup-devcontainer.ymlfor environment setup,publish-test-results.ymlfor publishing test results and coverage, andmerge-coverage.ymlfor merging and publishing code coverage reports. [1] [2] [3] [4] [5]GitHub Actions Pipeline Improvements
.github/workflows/ci.yamlis reworked to use matrix jobs for devcontainer-based testing, artifact upload for results and coverage, and a dedicated lint job. A new job aggregates and publishes combined test results and coverage reports, improving visibility and maintainability. [1] [2]Documentation and Configuration Updates
ci-tests.sh) are removed fromREADME.mdand.env.exampleis updated for clarity, reflecting the new devcontainer and pipeline workflow. [1] [2] [3]Platform Compatibility and Usability Enhancements
Does this introduce a breaking change?
Author pre-publish checklist
Pull Request Type
What kind of change does this Pull Request introduce?