Register crud module in pyproject.toml - #1231
Conversation
Add crud to packages list and telescope-crud entry point. Enables pip install telescope-benchmarks to include CRUD module.
|
For reviewers only: reply |
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.
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.
| return ready_nodes | ||
|
|
||
|
|
||
| def instrument_aks_nodepool_provisioning( |
There was a problem hiding this comment.
I don't see why we need this extra wrapper. The name is so close to instrument_nodepool_provisioning and this makes people confused. I don't see how this help generalize our codes.
There was a problem hiding this comment.
I added the wrapper when I added VirtualMachines support because the new path uses an Azure CLI callable while the existing VMSS path uses the SDK. I needed both operations to use the same ARM and Kubernetes readiness instrumentation.
Looking at the resulting call chain, I agree that the wrapper does not provide enough separation to justify another similarly named function. I removed it. AKSClient now builds the Kubernetes readiness callable and passes it, along with the prepared ARM callable, directly to instrument_nodepool_provisioning.
| if node_pool_type != "VirtualMachines": | ||
| parameters["count"] = node_count | ||
| parameters["vm_size"] = vm_size | ||
| return None |
There was a problem hiding this comment.
Here is the code smell:
Line 144 returns None but also mutate the parameters secretly, while line 147 returns a function. Check the call site - the None value of arm_callable is passed through but eventually fallback to begin_create_or_update_with_retry within provisioning_instrumentation.py.
How about something like this:
def prepare_create_operation(
parameters, node_pool_type, gpu_node_pool,
resource_group, cluster_name, node_pool_name, node_count, vm_size,
*, aks_client, k8s_client=None, # whatever the SDK callable needs
):
"""Return a zero-arg ARM callable for the create, dispatched by pool type."""
if node_pool_type == "VirtualMachines":
if gpu_node_pool:
raise ValueError("GPU node pools with type VirtualMachines are not supported")
return partial(
create_virtual_machines_node_pool,
resource_group, cluster_name, node_pool_name, node_count, vm_size,
)
# VMSS: still the SDK path, but returned as a callable instead of a None+mutation
parameters = {**parameters, "count": node_count, "vm_size": vm_size}
return partial(begin_create_or_update_with_retry,
aks_client, resource_group, cluster_name, node_pool_name, parameters)
to make it always return a callable.
The same reasoning can apply to def prepare_scale_operation below
There was a problem hiding this comment.
I agree that returning either None or a callable makes the contract difficult to follow. I originally used None to preserve the existing VMSS fallback while I added the VirtualMachines CLI path. I did not realize until your comment that this split the final operation selection between the preparation helper and the instrumentation wrapper.
I updated both prepare_create_operation and prepare_scale_operation to always return a zero-argument ARM callable. VMSS now returns a callable around begin_create_or_update_with_retry, while VirtualMachines returns the Azure CLI callable.
I also moved the GPU profile setup before the VMSS parameter copy and preserved the progressive-scale label. The tests now execute the VMSS callables and verify the SDK payloads, including progressive counts [2, 3].
| enable_managed_gpu: bool = False, | ||
| gpu_instance_profile: Optional[str] = None, | ||
| gpu_mig_strategy: Optional[str] = None, | ||
| node_pool_type: str = "VirtualMachineScaleSets", |
There was a problem hiding this comment.
Can we make VirtualMachineScaleSets as constant? I see multiple places where we have this hard-coded string.
There was a problem hiding this comment.
I agree that this value should not be repeated across the different layers. I added AzureNodePoolTypeConstants.VIRTUAL_MACHINE_SCALE_SETS and used it for the production defaults and comparisons in AKSClient, Azure NodePoolCRUD, and the CLI dispatch.
|
|
||
| def get_node_pool_scale_state(node_pool, node_pool_type): | ||
| """Return current node count and VM size for VMSS or VirtualMachines pools.""" | ||
| if node_pool_type != "VirtualMachines": |
There was a problem hiding this comment.
Can we make VirtualMachines as constant? Can you inspect all occurrences of this hard-coded str from this PR and update them
There was a problem hiding this comment.
I checked all occurrences of the hard-coded VirtualMachines value in my changes. I replaced the production comparisons, defaults, and Azure command construction with AzureNodePoolTypeConstants.VIRTUAL_MACHINES.
I kept the literal in error messages, docstrings, and the test that verifies the exact Azure CLI command because those are user-facing text or external-contract assertions.
| **azure_kwargs, | ||
| } | ||
| if args.cloud == "azure": | ||
| create_kwargs["node_pool_type"] = args.node_pool_type |
There was a problem hiding this comment.
There are many occurences like line131-132. You can just fold this to
azure_kwargs = {
"gpu_instance_profile": args.gpu_instance_profile,
"gpu_mig_strategy": args.gpu_mig_strategy,
"node_pool_type": args.node_pool_type,
}
There was a problem hiding this comment.
I agree that node_pool_type belongs with the other Azure-only arguments. I added it to the shared azure_kwargs dictionary and removed the repeated assignments from the create, scale, and all branches.
I scoped the dictionary to those three commands because delete uses the same handler but does not define node_pool_type, gpu_instance_profile, or gpu_mig_strategy. This keeps the shared construction without breaking Azure delete or forwarding Azure-only arguments to AWS.
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.exclude_managed_identitysupport toAKSClientand expose it as an opt-in CRUD CLI flag.NodePoolCRUDconstructor contract.Related PR
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
westcentralus,westus2,eastasia,eastus2euap, andcentraluseuap.c18584d6.3e335184.Review and Merge Sequence
main.mainfrom a disposable validation branch.Final Follow-up Validation
9f96c3e0.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.Final 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.