Skip to content

Register crud module in pyproject.toml - #1231

Open
Diamond (diamondpowell) wants to merge 12 commits into
mainfrom
dipowell/register-crud-package
Open

Register crud module in pyproject.toml#1231
Diamond (diamondpowell) wants to merge 12 commits into
mainfrom
dipowell/register-crud-package

Conversation

@diamondpowell

@diamondpowell Diamond (diamondpowell) commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Registers the open-source CRUD module as part of the telescope-benchmarks Python package so callers can install and run CRUD operations without source-tree PYTHONPATH routing.

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

  • Register crud, crud.azure, and crud.aws in pyproject.toml.
  • Add the telescope-crud console entry point.
  • Include deployment, Job, and StatefulSet YAML manifests in the installed package.
  • Consume the credential contract merged in PR #1305: shared configure_credential handling and explicit credential injection into AKSClient.
  • Keep Azure credential creation in the CRUD callers and expose managed-identity exclusion as an opt-in CLI flag.
  • Add optional AKS cluster-name selection while preserving discovery when omitted.
  • Preserve the existing positional NodePoolCRUD constructor contract.
  • Add focused tests for authentication behavior and constructor compatibility.

Related PRs

Credential refactor: PR #1305 merged on September 2, 2026 at merge commit 6b9fbaad7ae092d7b96fe7451f6b7987de314d22. This PR is rebased onto that contract at d41f41bf3536a0c1c4145c3aea127c4d67e10c61.

Internal migration: ADO PR 16193735

The two PRs are separated by ownership boundary:

  • This PR owns the reusable open-source Python package and runtime contract.
  • The internal PR owns pipeline orchestration, cloud login, resource validation, result publication, and production rollout.

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/python with 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.

    • Five of five Azure regions passed: westcentralus, westus2, eastasia, eastus2euap, and centraluseuap.
    • No failed or canceled records.
    • Internal commit: c18584d6.
    • Open-source package commit: 3e335184.

Post-Refactor new-pipeline-test.yml Validation

  • ADO build 179423290 passed using the temporary new-pipeline-test.yml configuration at internal test commit b9460721.
  • The run installed and validated open-source package commit d41f41bf, which includes the merged PR Refactor AKS client credential configuration #1305 credential refactor.
  • Cleanup commit 3730f654 restored new-pipeline-test.yml after validation; the temporary pipeline YAML is not part of this open-source PR.

Review and Merge Sequence

  1. Review this package PR and internal PR #16193735 in parallel.
  2. Merge this package PR first. Package registration remains separate and is not blocked on workload timing.
  3. Review and merge the planned workload-timing PR against the updated open-source main.
  4. Revalidate the already-reviewed internal migration against the resulting open-source main from a disposable validation branch.
  5. Merge internal PR #16193735 last, after required workload telemetry and consumer compatibility pass.
  6. Run a post-merge production smoke test and validate ADX output.

Final Follow-up Validation

  • Earlier open-source commit: 9f96c3e0; superseded by post-refactor validation at d41f41bf.
  • 123 focused tests passed after the VirtualMachines scale-state, exit-code, and helper/test extraction changes.
  • Pylint: 10.00/10.
  • Final file sizes: aks_client.py 974 lines, test_aks_client.py 838 lines, and test_main.py 1564 lines.
  • Targeted VirtualMachines lifecycle: ADO build 177546874.
    • Internal test commit: 00c99b90.
    • Open-source package commit: 9f96c3e0.
    • Create, scale up, Deployment, StatefulSet, Job, scale down, delete, collect, upload, and cleanup all passed.
    • No failed, canceled, or succeeded-with-issues records.

Build 177409535 remains the five-region VMSS installation validation; build 177546874 is the targeted VirtualMachines validation.

Earlier Authoritative Multi-Region Validation

  • Earlier Build: ADO build 177701638 (Aug 21, 2026)
  • Commits: Internal test commit b9460721 and open-source tested commit 9f96c3e0. Internal cleanup commit 3730f654 restored new-pipeline-test.yml (cleanup commit itself was not tested by build).
  • Regions: westcentralus, westus2, eastasia, eastus2euap, and centraluseuap. Both small VMSS and VirtualMachines cases ran in every region; 5/5 stages and 10/10 matrix jobs passed.
  • Scope: Package install, create, scale up, Deployment, StatefulSet, Job, scale down, delete, collect/upload/cleanup passed.
  • Status: No failed, canceled, abandoned, or succeeded-with-issues records.
  • Distinction: Clearly retain build 177409535 as earlier five-region VMSS package-installation evidence and build 177546874 as earlier one-region targeted VirtualMachines evidence; do not call either final-head all-type regional evidence.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

For reviewers only: reply /run-tf-integration to trigger the terraform integration pipeline before approving the PR.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) to pyproject.toml, include workload manifest templates as package data, and add the telescope-crud entry point.
  • Azure auth + selection: plumb cluster_name and exclude_managed_identity from CLI → NodePoolCRUDAKSClient (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.

Comment thread modules/python/crud/main.py Outdated
Comment thread modules/python/utils/provisioning_instrumentation.py Outdated
Comment thread modules/python/utils/azure_node_pool_cli.py Outdated
Comment thread modules/python/clients/aks_client.py Outdated
Comment thread modules/python/utils/azure_node_pool_cli.py Outdated
Comment thread modules/python/crud/main.py Outdated
Comment thread modules/python/clients/aks_client.py Outdated
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.
@diamondpowell
Diamond (diamondpowell) force-pushed the dipowell/register-crud-package branch from bbb5bf4 to d41f41b Compare September 2, 2026 21:37
str(node_count),
"--node-vm-size",
vm_size,
"--vm-set-type",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants