Repository navigation
Register crud module in pyproject.toml - #1231
Merged
Merged
Conversation
|
For reviewers only: reply |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the crud module installable as part of the telescope-benchmarks Python package and exposes it via a telescope-crud console entry point, while extending Azure CRUD plumbing to support explicit AKS cluster selection and optionally excluding managed identity from the Azure credential chain.
Changes:
- Package registration: add
crud(and subpackages) topyproject.toml, include workload manifest templates as package data, and add thetelescope-crudentry point. - Azure auth + selection: plumb
cluster_nameandexclude_managed_identityfrom CLI →NodePoolCRUD→AKSClient(DefaultAzureCredential). - Tests: add focused unit tests for the new credential-chain behavior and to ensure positional constructor compatibility.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| modules/python/pyproject.toml | Registers crud packages, includes workload template YAMLs, and adds telescope-crud console script. |
| modules/python/crud/main.py | Adds --cluster-name and --exclude-managed-identity CLI flags and passes them into Azure NodePoolCRUD. |
| modules/python/crud/azure/node_pool_crud.py | Extends NodePoolCRUD constructor to accept and forward cluster_name / exclude_managed_identity. |
| modules/python/clients/aks_client.py | Adds exclude_managed_identity to optionally exclude ManagedIdentityCredential from DefaultAzureCredential. |
| modules/python/tests/crud/test_azure_node_pool_crud.py | Adds unit tests validating auth configuration defaults and positional-arg compatibility. |
| modules/python/tests/clients/test_aks_client.py | Adds tests for DefaultAzureCredential default behavior vs. excluding managed identity. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Diamond (diamondpowell)
added a commit
that referenced
this pull request
Sep 2, 2026
## Context This refactor separates Azure credential construction from `AKSClient` so callers can explicitly provide the credential they need while the CRUD wrappers use one shared configuration path. It enables the workload CRUD package and pipelines to authenticate without relying on credentials created implicitly inside the client. After this merges, [PR #1231](#1231) will be updated to consume the new `AKSClient` credential contract. PR #1305 must merge first. ## Summary - move Azure credential construction into a shared `configure_credential` helper - require callers to inject credentials into `AKSClient` - update node pool and Machine API wrappers and tests for the new dependency boundary ## Validation - 782 tests passed, 1 skipped; coverage 82.91%; pylint 10.00/10 - [Open-source pipeline build 78625](https://akstelescope.visualstudio.com/telescope/_build/results?buildId=78625&view=results) passed - Node pool CRUD: create, scale up, scale down, and delete succeeded - Machine API: agent pool creation succeeded and all 10 scaled machines reached P100 readiness
Add crud to packages list and telescope-crud entry point. Enables pip install telescope-benchmarks to include CRUD module.
crud/azure was missing __init__.py, and crud.azure and crud.aws sub-packages were not listed in pyproject.toml packages. After pip install, imports like 'from crud.azure.node_pool_crud import NodePoolCRUD' would fail with ModuleNotFoundError.
Diamond (diamondpowell)
force-pushed
the
dipowell/register-crud-package
branch
from
September 2, 2026 21:37
bbb5bf4 to
d41f41b
Compare
Liyu Ma (liyu-ma)
approved these changes
Sep 7, 2026
This was referenced Sep 9, 2026
Xu Xue (xuexu6666)
added a commit
that referenced
this pull request
Sep 10, 2026
…crud.main) (#1319) > **Regression introduced by #1231** ("Register crud module in pyproject.toml"), which added `modules/python/crud/azure/__init__.py`. This PR fixes the fallout. ## Problem Every pipeline that executes the `crud` module has failed on every run since #1231 merged. The CRUD tasks fail immediately: ``` File ".../modules/python/crud/main.py", line 19 from crud.azure.node_pool_crud import NodePoolCRUD File ".../modules/python/clients/aks_client.py", line 20 from azure.core.exceptions import HttpResponseError, ResourceNotFoundError ModuleNotFoundError: No module named 'azure.core' ``` `azure-core` **is** installed correctly by "Install Dependencies" — this is namespace shadowing, not a missing dependency. ### Impacted pipelines (both crud engines) | Topology | Engine steps | Pipeline | Last green → now | |---|---|---|---| | `k8s-crud-gpu` | `engine/crud/k8s/{execute,collect}.yml` | GPU Cluster CRUD | green on `65836724`, red on every `c66241e4` run | | `k8s-cluster-crud-machine` | `engine/machine/k8s/{execute,collect}.yml` | Autoscale / Machine-API CRUD | green on `65836724`, red on every `c66241e4` run | A full-repo grep for `crud/main.py` finds only these four step files, so this PR covers 100% of the impacted call sites. ## Root cause The steps run the script as a **file**: ```yaml workingDirectory: modules/python env: PYTHON_SCRIPT_FILE: $(Pipeline.Workspace)/s/modules/python/crud/main.py script: python3 "$PYTHON_SCRIPT_FILE" create ... ``` Running `python3 .../crud/main.py` puts the script's own directory — `modules/python/crud/` — at `sys.path[0]`. #1231 added `modules/python/crud/azure/__init__.py`, turning `crud/azure/` into a **regular package**. Found first on `sys.path`, it shadows the installed `azure` SDK namespace package, so `azure.core` can no longer be resolved. Unit tests didn't catch this because pytest runs with a different `sys.path` that never places `crud/` at `sys.path[0]`. ## Fix Invoke the module with `-m` instead of running the file. `python3 -m crud.main` puts `modules/python` (the working directory) on `sys.path[0]` instead of `crud/`, so the `azure` SDK resolves from site-packages while `crud.azure` still imports. This is also what the `telescope-crud` entry point registered in #1231 was intended for. The now-unused `PYTHON_SCRIPT_FILE` env var is removed. Applied to all affected engine steps: - `steps/engine/crud/k8s/execute.yml` - `steps/engine/crud/k8s/collect.yml` - `steps/engine/machine/k8s/execute.yml` - `steps/engine/machine/k8s/collect.yml` ## Verification Pipeline runs on this branch (`xuxue/crud-azure-namespace-shadowing`): - **GPU Cluster CRUD** — [build 79586](https://dev.azure.com/akstelescope/telescope/_build/results?buildId=79586&view=results) - **Autoscale / Machine-API CRUD** — [build 79591](https://dev.azure.com/akstelescope/telescope/_build/results?buildId=79591&view=results) Both run the actual CRUD create/scale/delete operations — sailing past the `ModuleNotFoundError` that killed every prior run. Also reproduced the shadowing and confirmed the fix locally: ``` # crud/ on sys.path[0] (file invocation) import azure.core.exceptions -> ModuleNotFoundError: No module named 'azure.core' # modules/python on sys.path[0] (-m crud.main) import azure.core.exceptions; import crud.azure -> both import OK ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Registers the open-source CRUD module as part of the
telescope-benchmarksPython package so callers can install and run CRUD operations without source-treePYTHONPATHrouting.Why
The internal Telescope CRUD migration now uses the open-source Python CRUD module as the source of truth for node-pool and workload operations. This PR provides the package contract required by that migration: installable modules, runtime workload manifests, a CLI entry point, and explicit pipeline authentication and cluster selection inputs.
Changes
crud,crud.azure, andcrud.awsinpyproject.toml.telescope-crudconsole entry point.configure_credentialhandling and explicit credential injection intoAKSClient.NodePoolCRUDconstructor contract.Related PRs
Credential refactor: PR #1305 merged on September 2, 2026 at merge commit
6b9fbaad7ae092d7b96fe7451f6b7987de314d22. This PR is rebased onto that contract atd41f41bf3536a0c1c4145c3aea127c4d67e10c61.Internal migration: ADO PR 16193735
The two PRs are separated by ownership boundary:
The internal PR is intentionally open as this package's integration consumer. It shows reviewers how the package is installed and invoked, so review of both PRs can happen in parallel. The internal PR does not need to merge before this package PR.
The internal validation pipeline cloned this draft branch directly and installed
modules/pythonwith pip. Draft status does not affect branch installation as long as the branch is pushed.Validation
Final open-source head:
d41f41bf3536a0c1c4145c3aea127c4d67e10c61.797 tests passed, 1 skipped; coverage 82.99%; Pylint 10.00/10.
GitHub Python Validation, YAML Validation, post-comment, and CLA checks passed.
100 focused CRUD and AKS client unit tests passed before pipeline validation.
81 focused CRUD tests passed after the constructor-compatibility fix.
Python Validation: passed.
YAML Validation: passed.
CLA: passed.
Feature-to-feature pip-install validation: ADO build 177409535.
westcentralus,westus2,eastasia,eastus2euap, andcentraluseuap.c18584d6.3e335184.Post-Refactor
new-pipeline-test.ymlValidationnew-pipeline-test.ymlconfiguration at internal test commitb9460721.d41f41bf, which includes the merged PR Refactor AKS client credential configuration #1305 credential refactor.3730f654restorednew-pipeline-test.ymlafter validation; the temporary pipeline YAML is not part of this open-source PR.Review and Merge Sequence
main.mainfrom a disposable validation branch.Final Follow-up Validation
9f96c3e0; superseded by post-refactor validation atd41f41bf.10.00/10.aks_client.py974 lines,test_aks_client.py838 lines, andtest_main.py1564 lines.00c99b90.9f96c3e0.Build
177409535remains the five-region VMSS installation validation; build177546874is the targeted VirtualMachines validation.Earlier Authoritative Multi-Region Validation
b9460721and open-source tested commit9f96c3e0. Internal cleanup commit3730f654restorednew-pipeline-test.yml(cleanup commit itself was not tested by build).westcentralus,westus2,eastasia,eastus2euap, andcentraluseuap. Both small VMSS and VirtualMachines cases ran in every region; 5/5 stages and 10/10 matrix jobs passed.177409535as earlier five-region VMSS package-installation evidence and build177546874as earlier one-region targeted VirtualMachines evidence; do not call either final-head all-type regional evidence.