Skip to content

Register crud module in pyproject.toml - #1231

Merged
Diamond (diamondpowell) merged 12 commits into
mainfrom
dipowell/register-crud-package
Sep 8, 2026
Merged

Diamond (diamondpowell) merged 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 → 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.

Comment thread modules/python/crud/main.py Resolved
Comment thread modules/python/clients/aks_client.py Resolved
Comment thread modules/python/crud/main.py Resolved
Comment thread modules/python/clients/aks_client.py Resolved
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
@diamondpowell
Diamond (diamondpowell) merged commit c66241e into main Sep 8, 2026
4 checks passed
@diamondpowell
Diamond (diamondpowell) deleted the dipowell/register-crud-package branch September 8, 2026 14:03
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
```
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