Register crud module in pyproject.toml - #1231
Conversation
|
For reviewers only: reply |
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.
## 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.
bbb5bf4 to
d41f41b
Compare
| str(node_count), | ||
| "--node-vm-size", | ||
| vm_size, | ||
| "--vm-set-type", |
There was a problem hiding this comment.
Why do we need to add --vm-set-type for GPU pool? This is a behavior change different from current implementation:
https://github.com/Azure/telescope/blob/main/modules/python/clients/aks_client.py#L289-L320
Will this break things? Xu Xue (@xuexu6666)
There was a problem hiding this comment.
Ahhh it isn’t needed for the managed GPU path. I accidentally passed it through as well while adding VirtualMachines support to the non-GPU path. I removed it from the managed GPU command and added regression coverage to keep the paths separate.
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.