Skip to content

Conversation

@simonjj
Copy link

@simonjj simonjj commented Nov 13, 2025

Related command

az containerapp compose ...

General Guidelines

  • [Y] Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
===============
| Style Check |
===============

Modules: containerapp

Running pylint on modules...
Pylint: PASSED

Running flake8 on modules...
Flake8: PASSED
  • [Y] Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
----------------------------------------------------------------------
Ran 9 tests in 0.187s

OK (skipped=2)

Simon Jakesch added 2 commits November 12, 2025 17:33
- Parse 'models:' section from compose files with runtime_flags support
- Deploy models container app with GPU workload profile support
- Inject MODEL_RUNNER_URL and MODEL_RUNNER_MODEL into consuming services
- Add MCP gateway detection and automatic configuration
- Support x-azure-deployment overrides for ingress and resources
- Add --dry-run flag for deployment preview without creating resources
- Add --replace-all flag to replace all existing apps in environment
…mprovements

- Auto-detect GPU requirements and select appropriate workload profiles
- Prefer T4 GPU over A100 for cost optimization
- Default to internal ingress for models and mcp-gateway services
- Support image override via x-azure-deployment.image
- Add allowInsecure ingress option for development scenarios
- Fix model endpoint variable injection (use getattr for safe access)
- Fix MODELS_CONFIG schema to properly include runtime_flags
- Fix dynamic subscription ID in role assignments
- Remove azure-cli core dependency requirements (extension is self-contained)
Copilot AI review requested due to automatic review settings November 13, 2025 00:07
@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Nov 13, 2025

⚠️Azure CLI Extensions Breaking Change Test
⚠️containerapp
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd containerapp compose create cmd containerapp compose create added parameter dry_run
⚠️ 1006 - ParaAdd containerapp compose create cmd containerapp compose create added parameter replace_all

@yonzhan
Copy link
Collaborator

yonzhan commented Nov 13, 2025

Thank you for your contribution! We will review the pull request and get back to you soon.

@github-actions
Copy link

The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR.

Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions).
After that please run the following commands to enable git hooks:

pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>

@github-actions
Copy link

CodeGen Tools Feedback Collection

Thank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey

Copilot finished reviewing on behalf of simonjj November 13, 2025 00:09
@github-actions
Copy link

github-actions bot commented Nov 13, 2025

Hi @simonjj

Release Suggestions

Module: containerapp

  • Please log updates into to src/containerapp/HISTORY.rst
  • Update VERSION to 1.3.0b2 in src/containerapp/setup.py

Notes

Copy link
Contributor

Copilot AI left a comment

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 adds comprehensive "compose-for-agents" support to the Azure Container Apps extension, enabling deployment of AI/ML workloads with models, MCP gateways, and GPU workload profiles through Docker Compose files.

Key Changes:

  • Models section support with automatic GPU workload profile provisioning and model-runner container deployment
  • MCP gateway auto-detection, managed identity enablement, and role assignment
  • Dry-run mode (--dry-run) for deployment preview without resource creation
  • x-azure-deployment custom extension for resource overrides (CPU, memory, ingress, images)

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
setup.py Version bump to 1.2.0b6
HISTORY.rst Detailed changelog documenting new compose features
_params.py Added --dry-run and --replace-all parameters
_constants.py Added model runner and MCP gateway default images
custom.py Major implementation: models deployment, MCP gateway config, environment injection, dry-run mode, and declarative updates
_compose_utils.py Core utilities: GPU profile management, models container creation, MCP gateway helpers, x-azure-deployment parsing, and dry-run functions
_compose_ported.py Helper functions ported from azure-cli core for models and MCP gateway parsing
test_containerapp_compose_models.py Test suite for models deployment scenarios
test_containerapp_compose_mcp_gateway.py Test suite for MCP gateway detection and configuration

return containerapps_from_compose

# Return empty list (no actual deployment)
return None # Dry-run complete, no resources created return containerapps_from_compose
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

This line contains a syntax error with malformed text at the end. The line should end with return containerapps_from_compose but instead has return None # Dry-run complete, no resources created return containerapps_from_compose which mixes multiple return statements and a comment on a single line. This will cause a syntax error.

Suggested change
return None # Dry-run complete, no resources created return containerapps_from_compose
return None # Dry-run complete, no resources created

Copilot uses AI. Check for mistakes.
"target", "secrets", "tags"]
if compose_service.build is not None:
config_list = resolve_configuration_element_list(compose_service, unsupported_configuration, 'build')
message = "These build configuration settings from the docker-compose file are yet supported."
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The message text contains "are yet supported" which is missing a word. This should read "are not yet supported" to be grammatically correct.

Suggested change
message = "These build configuration settings from the docker-compose file are yet supported."
message = "These build configuration settings from the docker-compose file are not yet supported."

Copilot uses AI. Check for mistakes.
Comment on lines 2142 to 2143
# if service_name == 'models':
# dry_run_has_models = True
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The commented out code # if service_name == 'models': and # dry_run_has_models = True should either be removed or uncommented if needed. Leaving commented-out code reduces maintainability and can cause confusion about intended behavior.

Suggested change
# if service_name == 'models':
# dry_run_has_models = True

Copilot uses AI. Check for mistakes.
Comment on lines 2073 to 2077
# Show allow insecure if target port is set
print(f" Allow Insecure: true")
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The dry-run print function always prints "Allow Insecure: true" when a target port is set (line 2074), but this should be conditional based on the actual allow_insecure configuration in the service. This creates misleading output that doesn't match the actual configuration that would be deployed.

Suggested change
# Show allow insecure if target port is set
print(f" Allow Insecure: true")
# Show allow insecure if present in config
if 'allow_insecure' in service_config:
print(f" Allow Insecure: {str(service_config['allow_insecure']).lower()}")

Copilot uses AI. Check for mistakes.
Comment on lines 317 to 324
if len(ports) > 1:
message = f"You have more than one {name} mapping defined in your docker-compose file."
message += " Which port would you like to use? "
choice_index = prompt_choice_list(message, ports)

return ports[choice_index]


Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The variable choice_index is used without being initialized. In the case where len(ports) > 1, the code calls prompt_choice_list but doesn't assign the result. The next line return ports[choice_index] will cause a NameError. This should be: choice_index = prompt_choice_list(message, ports).

Suggested change
if len(ports) > 1:
message = f"You have more than one {name} mapping defined in your docker-compose file."
message += " Which port would you like to use? "
choice_index = prompt_choice_list(message, ports)
return ports[choice_index]
if not ports:
raise ValueError(f"No {name} mappings defined.")
if len(ports) == 1:
return ports[0]
else:
message = f"You have more than one {name} mapping defined in your docker-compose file."
message += " Which port would you like to use? "
choice_index = prompt_choice_list(message, ports)
return ports[choice_index]

Copilot uses AI. Check for mistakes.
Comment on lines 1817 to 1846
import re
path_match = re.search(r'://mcp-gateway(?::\d+)?(/[^\s]*)', var_value)
preserved_path = path_match.group(1) if path_match else ''

env_var['value'] = gateway_fqdn + preserved_path
logger.info(f"Transformed {var_name} to FQDN with path: {env_var['value']}")
else:
# Match http://hostname:port or https://hostname:port patterns
env_var['value'] = re.sub(r'(https?://[^:/]+):\d+', r'\1', env_var['value'])
elif isinstance(env_var, str) and '=' in env_var:
# Handle string format "KEY=VALUE"
key, value = env_var.split('=', 1)

if key in mcp_gateway_vars and '://mcp-gateway' in value:
from ._compose_utils import get_containerapp_fqdn
gateway_external = False
for svc_name in parsed_compose_file.services.keys():
if detect_mcp_gateway_service(svc_name, parsed_compose_file.services[svc_name]):
raw_gateway_yaml = compose_yaml_original.get('services', {}).get(svc_name, {})
gateway_overrides = parse_x_azure_deployment(raw_gateway_yaml)
if gateway_overrides.get('ingress_type') == 'external':
gateway_external = True
break

gateway_fqdn = get_containerapp_fqdn(cmd, resource_group_name, managed_env_name, 'mcp-gateway', is_external=gateway_external)
gateway_protocol = 'http' if gateway_allow_insecure else 'https'
gateway_fqdn = gateway_fqdn.replace('https://', f'{gateway_protocol}://')

# Extract and preserve the path from the original URL
import re
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The import re statement is duplicated - it appears once at line 1761 and again at line 1817 and 1846. The module is already imported earlier in the conditional block, so these additional imports are redundant and should be removed.

Copilot uses AI. Check for mistakes.
replicas=replicas,
is_models_service=(service_name == 'models'),
is_mcp_gateway=is_mcp_gateway,
gpu_type=gpu_profile.get('type') if (service_name == 'models' and 'gpu_profile' in locals()) else None,
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The variable gpu_profile is referenced but never defined in this scope. The code checks if (service_name == 'models' and 'gpu_profile' in locals()) but gpu_profile is only assigned if models exist (line 1629-1664). If dry_run is True and this code path is reached for a non-models service, this will cause a NameError. Consider using gpu_profile_name instead, or ensure proper initialization.

Copilot uses AI. Check for mistakes.
Comment on lines +2125 to +2124
service_name=service_name,
service=service,
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The condition if min_replicas is not None will also handle the case when min_replicas is 0, which is a valid value. However, the subsequent check in the dry-run section may not handle this correctly. Consider explicitly checking for integers greater than or equal to 0 to make the intent clearer.

Copilot uses AI. Check for mistakes.
Comment on lines +760 to +764
except Exception as e:
logger.warning(f"Failed to check existing profiles: {str(e)}")
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The exception handling catches a generic Exception, which is overly broad. Consider catching specific exceptions like ResourceNotFoundError or HttpResponseError from Azure SDK to provide more targeted error handling and avoid masking unexpected errors.

Copilot uses AI. Check for mistakes.
from azure.core.exceptions import ResourceNotFoundError

try:
existing_app = ContainerAppClient.show(cmd, resource_group_name=resource_group_name, name=app_name)
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The code uses from ._clients import ContainerAppClient but this import appears inside a function. If ContainerAppClient is not available in the module scope at line 1875, this will cause an import error. Consider importing at the module level or handling the import error appropriately.

Copilot uses AI. Check for mistakes.
@Greedygre
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Greedygre
Copy link
Contributor

Please fix the CI style failed

upcoming
++++++

1.2.0b6
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't update the version directly, we will release it in a separate PR.

self.assertIn('http://models', env_vars['MODELS_ENDPOINT'])

@ResourceGroupPreparer(location='westus3')
@live_only()
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why these tests all mark as live_only?

# HISTORY.rst entry.

VERSION = '1.2.0b5'
VERSION = '1.2.0b6'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't update version in this PR, we will kick a release when ready

Comment on lines 524 to 532
with self.argument_context('containerapp env premium-ingress') as c:
c.argument('resource_group_name', arg_type=resource_group_name_type, id_part=None)
c.argument('name', options_list=['--name', '-n'], help="The name of the managed environment.")
c.argument('workload_profile_name', options_list=['--workload-profile-name', '-w'], help="The workload profile to run ingress replicas on. This profile must not be shared with any container app or job.")
c.argument('min_replicas', options_list=['--min-replicas'], type=int, deprecate_info=c.deprecate(hide=True, expiration='2.79.0'), help="The workload profile minimum instances is used instead.")
c.argument('max_replicas', options_list=['--max-replicas'], type=int, deprecate_info=c.deprecate(hide=True, expiration='2.79.0'), help="The workload profile maximum instances is used instead.")
c.argument('termination_grace_period', options_list=['--termination-grace-period', '-t'], type=int, help="Time in seconds to drain requests during ingress shutdown. Default 500, minimum 0, maximum 3600.")
c.argument('request_idle_timeout', options_list=['--request-idle-timeout'], type=int, help="Timeout in minutes for idle requests. Default 4, minimum 4, maximum 30.")
c.argument('header_count_limit', options_list=['--header-count-limit'], type=int, help="Limit of http headers per request. Default 100, minimum 1.")
Copy link
Contributor

@Greedygre Greedygre Nov 13, 2025

Choose a reason for hiding this comment

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

These params have been moved to azure-cli . They don't relate to this PR, right?

return (startup_command_array, startup_args_array)


# ============================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

If you only copy them from azure-cli, you can import them from azure-cli,for example:

from azure.cli.command_modules.containerapp._compose_utils import resolve_service_startup_command, resolve_ingress_and_target_port,

]


def should_deploy_model_runner(compose_yaml: Dict, parsed_compose_file) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any changes in these functions? Do you mean some of them changed and some are just copy?
Recommend to import for azure-cli directly

Expected: No actual Azure resources created, detailed report generated.
This test MUST FAIL until T048-T056 are implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is test MUSt fail until T048 - T056

- Fix syntax error: Remove duplicate return statement (line 2218)
- Fix grammar: 'are yet supported' -> 'are not yet supported'
- Remove commented-out code
- Fix dry-run allow_insecure to show actual config value
- Fix choice_index undefined variable with proper error handling
- Remove duplicate 'import re' statements
- Fix gpu_profile undefined variable by reading from raw YAML
- Revert version from 1.2.0b6 to 1.2.0b5 (maintainers handle releases)
- Remove premium-ingress params that came from upstream rebase

Addresses feedback from @Greedygre and Copilot AI review
@simonjj simonjj force-pushed the ai-compose-refactor branch from 4e3fcab to 004c93f Compare November 13, 2025 14:32
"""

@ResourceGroupPreparer(location='westus3') # GPU region
@live_only()
Copy link
Contributor

Choose a reason for hiding this comment

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

"""
self.kwargs.update({
'env_name': self.create_random_name(prefix='env', length=24),
'compose_file': os.path.join(os.path.dirname(__file__), '../../../../../../test-resources/compose-files/models-basic.yml')
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this file? How did you test this command? Did you test it locally?

Simon Jakesch added 7 commits November 13, 2025 11:37
- Fix undefined variables (update_containerapp_from_compose, ContainerAppClient)
- Remove 322 trailing whitespace errors across all Python files
- Fix 4 unused variable warnings (models_app, models_list, gateway_url)
- Convert f-strings without placeholders to regular strings
- Fix PEP8 formatting (spacing, indentation, blank lines)

Addresses CI test failures from PR Azure#9422 review feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto-Assign Auto assign by bot ContainerApp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants