Add backend-neutral runner and Azure ML compute backend - #192
Closed
Joaquín Rivero (jQuinRivero) wants to merge 18 commits into
Closed
Add backend-neutral runner and Azure ML compute backend#192Joaquín Rivero (jQuinRivero) wants to merge 18 commits into
Joaquín Rivero (jQuinRivero) wants to merge 18 commits into
Conversation
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
September 2, 2026 02:29
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
infra/modules/functions.bicep emits an empty AML_IDENTITY_MODE when AML is disabled, which can break later enablement by flipping AML_MODE without also updating identity mode, indicating a real configuration bug.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Introduces a backend-neutral compute execution contract in hastelib and wires an Azure Machine Learning (AML) adapter alongside existing Azure Batch and local Docker execution, with configuration, IaC, and documentation updates to support opt-in enablement and deterministic routing.
Changes:
- Add typed compute contracts (
ComputeJobSpec/ComputeJobHandle) and runner infrastructure (RunnerRegistry,ComputeRouter,ComputeExecutionService) to support Batch/AML/local backends. - Extend API + queue handlers to validate per-job backend selection, persist provider handles, and sanitize logging.
- Add AML “Existing/Create/Disabled” wiring across infra, dependency pinning, docs/spec/ADR, and a changelog entry.
File summaries
| File | Description |
|---|---|
| THIRD-PARTY-NOTICES.md | Notes Azure SDK usage now includes AML job submission (azure-ai-ml). |
| spec/features/aml-compute-backend/README.md | Feature spec overview, success criteria, and component impact map. |
| spec/features/aml-compute-backend/impact-analysis.md | Impact/risk/security analysis for the compute-layer refactor + AML backend. |
| spec/architecture/overview.md | Updates architecture overview to include AML/local as compute backends. |
| spec/architecture/decisions/0005-backend-neutral-compute-runner-and-aml-backend.md | ADR documenting the new compute contract and AML introduction strategy. |
| README.md | Updates top-level architecture diagram to reflect compute backend options. |
| infra/modules/functions.bicep | Emits compute default + AML app settings into Function Apps. |
| infra/modules/amlWorkspace.bicep | Create-mode AML workspace module (keyless, MI-based). |
| infra/modules/amlRole.bicep | Create-mode AML RBAC assignment module (least privilege). |
| infra/modules/amlEnvironment.bicep | Create-mode AML immutable environment version registration. |
| infra/modules/amlDatastore.bicep | Create-mode AML datastore registration using identity-based access. |
| infra/modules/amlCompute.bicep | Create-mode AML compute cluster provisioning (scale-to-zero). |
| infra/main.bicepparam | Adds env-driven parameters for compute default and AML modes/settings. |
| hastelib/tests/core/test_config.py | Unit tests for compute/AML config parsing and defaults. |
| hastelib/tests/core/runners/test_runner_log_safety.py | Regression tests to prevent signed URLs/resource dicts leaking to logs. |
| hastelib/tests/core/runners/test_registry.py | Unit tests for runner registry caching + lazy import behavior. |
| hastelib/tests/core/runners/test_azure_batch_job_execution_binding.py | Unit tests for idempotent Batch job binding + auto-terminate behavior. |
| hastelib/tests/core/processors/test_processors_backend_neutral.py | Enforces processor neutrality (no direct Batch config/vars; spec builders exist). |
| hastelib/tests/core/processors/test_imagery_preprocess_config.py | Updates imagery processor construction to use execution service injection. |
| hastelib/tests/core/processors/test_imagery_output_fallback.py | Migrates imagery output/log fallback tests to handle-based output reads. |
| hastelib/tests/core/models/test_projects_compute.py | Tests additive compute fields on project/job models. |
| hastelib/tests/core/models/init.py | Adds package marker for model tests. |
| hastelib/tests/build/test_release_workflows.py | Ensures deploy script emits compute + AML settings (drift prevention). |
| hastelib/tests/build/test_dependency_pins.py | Enforces azure-ai-ml==1.34.1 pinning and installation scope rules. |
| hastelib/src/hastegeo/core/runners/unified_runner.py | Deprecates legacy factory; improves log safety around resource upload dict. |
| hastelib/src/hastegeo/core/runners/router.py | Adds deterministic weighted rendezvous routing for auto backend. |
| hastelib/src/hastegeo/core/runners/registry.py | Adds adapter registry with caching + lazy import + classified errors. |
| hastelib/src/hastegeo/core/runners/execution_service.py | Adds submission orchestration + handle-based lifecycle dispatch. |
| hastelib/src/hastegeo/core/models/projects.py | Adds additive compute fields (computeJob, computeBackend) to models. |
| hastelib/pyproject.toml | Adds optional azure-ml extra and enables it for the test env. |
| env.yml | Adds azure-ai-ml==1.34.1 to dev environment dependencies. |
| docs/configuration.md | Documents compute backend selection and AML backend configuration/modes. |
| docs/architecture.md | Documents compute backends and new runner architecture at a high level. |
| docker/training/scripts/tests/test_set_dirs.py | Tests container workdir contract (HASTE_JOB_WORKDIR + legacy alias). |
| docker/training/scripts/tests/test_entrypoint_workdir.py | Tests entrypoint workdir resolution matches contract. |
| docker/training/scripts/tests/init.py | Adds package marker for training script tests. |
| docker/training/scripts/set_dirs.sh | Implements canonical HASTE_JOB_WORKDIR resolution + legacy aliasing. |
| docker/training/scripts/entrypoint.sh | Uses provider-neutral workdir for post-command chmod step. |
| docker/training/code/tests/test_run_workflow.py | Tests workdir resolution in training workflow runner code. |
| docker/training/code/run_workflow.py | Uses provider-neutral workdir resolution helper. |
| docker/imageryprep/scripts/tests/test_set_dirs.py | Tests imageryprep container workdir contract and substitution behavior. |
| docker/imageryprep/scripts/tests/init.py | Adds package marker for imageryprep script tests. |
| docker/imageryprep/scripts/set_dirs.sh | Implements canonical workdir resolution + legacy aliasing for imageryprep. |
| CHANGELOG.md | Adds changelog entry describing backend-neutral compute + AML backend. |
| api/hastefuncqueues/tests/test_compute_follow_on.py | Tests follow-on backend propagation and queue logging sanitization. |
| api/hastefuncqueues/requirements.txt | Adds azure-ai-ml==1.34.1 to the submitting function app dependency set. |
| api/hastefuncqueues/function_app.py | Sanitizes queue logs; wires follow-on backend propagation helpers. |
| api/hastefuncapi/function_app.py | Validates backend selection; protects server-owned runtime fields/handles. |
| .github/scripts/deploy_apps.sh | Emits compute + AML settings in legacy deploy path (drift parity). |
| .github/scripts/check_env_drift.py | Documents allowlist limitations for dynamically-read AML/compute settings. |
Review details
- Files reviewed: 63/84 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
September 2, 2026 02:55
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The legacy deploy script leaves AML_SUBSCRIPTION_ID empty by default even though the AML runner requires it when AML is enabled, making deploy_apps.sh-based AML enablement fail unless manually overridden.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 64/85 changed files
- Comments generated: 1
- Review effort level: Lite
RC artifacts readyAll branch deployment references use the same RC tag:
|
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
September 2, 2026 03:03
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It’s a broad, high-blast-radius compute/infra architecture change (new execution contract + routing + AML wiring) that warrants final human review despite strong test coverage.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
api/hastefuncapi/function_app.py:2450
- This 400 response doesn’t indicate which client-supplied fields triggered the rejection, making it hard to debug invalid requests. Since
supplied_nonempty_fields()already computedforbidden_fields, include it in the message.
hastelib/src/hastegeo/core/runners/unified_runner.py:36
- The DeprecationWarning message concatenates two string literals without a separating space/newline, producing
...execution_service.ComputeExecutionService...in the emitted warning text.
- Files reviewed: 64/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
RC artifacts readyAll branch deployment references use the same RC tag:
|
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
September 2, 2026 03:11
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
A few concrete correctness/maintainability issues remain (notably Bicep comment typos/misreferences and a potential lock-contention/deadlock risk in RunnerRegistry adapter construction).
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
hastelib/src/hastegeo/core/runners/registry.py:113
- RunnerRegistry.get() holds the global lock while calling the adapter factory (which may do slow/blocking work like connecting to Docker). This can serialize concurrent submissions and can deadlock if an adapter constructor (directly or indirectly) calls back into the registry. Consider retrieving the factory under the lock, constructing outside the lock, then re-acquiring the lock to cache the instance with a double-check.
- Files reviewed: 64/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
RC artifacts readyAll branch deployment references use the same RC tag:
|
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
September 2, 2026 03:32
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large, cross-cutting compute-architecture refactor plus new AML backend + IaC wiring, which warrants careful human validation of operational behavior and rollout safety.
Review details
- Files reviewed: 63/85 changed files
- Comments generated: 0 new
- Review effort level: Lite
RC artifacts readyAll branch deployment references use the same RC tag:
|
RC artifacts readyAll branch deployment references use the same RC tag:
|
RC artifacts readyAll branch deployment references use the same RC tag:
|
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
September 2, 2026 11:24
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The Actions deployment cannot receive AML settings, and Create mode omits required identity access to workspace dependencies.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/scripts/deploy_apps.sh:70
- The standard deploy workflow never exports
COMPUTE_BACKEND_DEFAULTor anyAML_*GitHub Environment value into this script;.github/workflows/deploy-apps.yml:110-145only maps publishing and Batch settings. Consequently Actions deployments always take theazure_batch/Disableddefaults here, so the documented Existing-mode configuration cannot be enabled through the normal workflow. Add the correspondingvars.*/secrets.*mappings to the Deploy step.
- Files reviewed: 63/85 changed files
- Comments generated: 3
- Review effort level: Balanced
11 tasks
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.
Description
Adds a typed, backend-neutral compute execution contract and Azure Machine Learning command-job adapter while preserving Azure Batch and local Docker behavior.
ComputeJobHandlelifecycle operations.autobackend routing, idempotent submission, legacy Batch-handle compatibility, and server-owned runtime-field protection.HASTE_JOB_WORKDIRthe provider-neutral container workspace contract while retaining legacy Batch aliases.The initial rollout references operator-provided AML resources with
AML_MODE=Existing. No Azure resources were created, changed, or deployed during implementation or validation.Type of change
Checklist
cd hastelib && hatch run test:pytest) and the UI lints clean (cd ui && npm run lint)Testing
hastelibsuite: 1,091 passed, 5 skipped, with one unchanged Azurite-dependent fixture failure intest_artifacts.py::TestArtifactProcessor::test_zip; the fixture fails while creating its test container before feature code runs because local Azurite is unavailable.infra/main.jsonis synchronized; static checks confirm Existing mode creates or mutates zero AML resources.Live AML CPU/GPU smoke tests, five-workload Batch/AML output-parity tests, mixed-backend execution, cross-deployment access denial, and live deployment validation remain Stage 1 rollout gates. They were not run because cloud operations were explicitly excluded from this implementation pass.
Additional context
The commit history is split into atomic commits covering the spec/ADR, core contract, Batch/local adapters, AML adapter/dependency, workload migration, container workspace contract, API/queue integration, infrastructure wiring, and public documentation.