Skip to content

Conversation

@kshitij-microsoft
Copy link
Member

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

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 pull request adds comprehensive end-to-end (E2E) tests for Deployment Templates functionality in Azure ML SDK. The PR introduces test infrastructure, YAML configuration files, and test cases covering basic to advanced deployment template scenarios.

Changes:

  • Added three YAML configuration files for different test scenarios (minimal, basic, complete)
  • Implemented main E2E test file with core operations (CRUD, archive/restore, error handling)
  • Implemented advanced E2E test file with complex scenarios (probes, request settings, version management, concurrent operations)
  • Added conftest.py with shared fixtures for test setup

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
sdk/ml/azure-ai-ml/tests/test_configs/deployment_template/minimal_deployment_template.yaml Minimal deployment template configuration with required fields only
sdk/ml/azure-ai-ml/tests/test_configs/deployment_template/complete_deployment_template.yaml Complete deployment template with all available features and settings
sdk/ml/azure-ai-ml/tests/test_configs/deployment_template/basic_deployment_template.yaml Basic deployment template with common configuration options
sdk/ml/azure-ai-ml/tests/deployment_template/e2etests/test_deployment_template_advanced.py Advanced E2E tests covering complex scenarios like probes, version management, and concurrent operations
sdk/ml/azure-ai-ml/tests/deployment_template/e2etests/test_deployment_template.py Core E2E tests for CRUD operations, listing, archiving, and error handling
sdk/ml/azure-ai-ml/tests/deployment_template/e2etests/conftest.py Shared pytest fixtures for deployment template tests
sdk/ml/azure-ai-ml/tests/deployment_template/e2etests/init.py Package initialization file with copyright header

Comment on lines +45 to +46
except Exception:
pass # Template doesn't exist, continue
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The 'cleanup_template' function is duplicated in both test_deployment_template.py and test_deployment_template_advanced.py. This should be moved to a shared utility module (e.g., conftest.py or a test utilities module) to avoid code duplication.

Suggested change
except Exception:
pass # Template doesn't exist, continue
except Exception as ex: # pylint: disable=broad-exception-caught
# Template doesn't exist or could not be deleted; ignore but log for visibility
logger.info(
"Cleanup skipped or failed for template '%s' version '%s': %s",
name,
version,
ex,
)

Copilot uses AI. Check for mistakes.
) -> None:
"""Test creating a deployment template with complex environment variables.

Args:`n registry_client: ML registry client fixture
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The docstring contains a malformed "Args:" line with backtick-n sequence. This should be properly formatted as "Args:" on its own line.

Copilot uses AI. Check for mistakes.
) -> None:
"""Test managing multiple versions of the same deployment template.

Args:`n registry_client: ML registry client fixture
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The docstring contains a malformed "Args:" line with backtick-n sequence. This should be properly formatted as "Args:" on its own line.

Suggested change
Args:`n registry_client: ML registry client fixture
Args:
registry_client: ML registry client fixture

Copilot uses AI. Check for mistakes.
) -> None:
"""Test deployment template with various tag combinations.

Args:`n registry_client: ML registry client fixture
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The docstring contains a malformed "Args:" line with backtick-n sequence. This should be properly formatted as "Args:" on its own line.

Suggested change
Args:`n registry_client: ML registry client fixture
Args:
registry_client: ML registry client fixture

Copilot uses AI. Check for mistakes.
) -> None:
"""Test creating a deployment template with custom request settings.

Args:`n registry_client: ML registry client fixture
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The docstring contains a malformed "Args:" line with backtick-n sequence. This should be properly formatted as "Args:" on its own line.

Suggested change
Args:`n registry_client: ML registry client fixture
Args:
registry_client: ML registry client fixture

Copilot uses AI. Check for mistakes.
) -> None:
"""Test updating a minimal template to a full-featured template.

Args:`n registry_client: ML registry client fixture
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The docstring contains a malformed "Args:" line with backtick-n sequence. This should be properly formatted as "Args:" on its own line.

Copilot uses AI. Check for mistakes.
) -> None:
"""Test creating and managing concurrent versions of deployment templates.

Args:`n registry_client: ML registry client fixture
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The docstring contains a malformed "Args:" line with backtick-n sequence. This should be properly formatted as "Args:" on its own line.

Suggested change
Args:`n registry_client: ML registry client fixture
Args:
registry_client: ML registry client fixture

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +14
from azure.ai.ml.entities import Environment


@pytest.fixture(scope="session")
def test_environment(registry_client: MLClient) -> str:
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The import 'Environment' is unused and should be removed. It appears to be left over from development but is not used anywhere in this file.

Suggested change
from azure.ai.ml.entities import Environment
@pytest.fixture(scope="session")
def test_environment(registry_client: MLClient) -> str:
@pytest.fixture(scope="session")
def test_environment(registry_client: MLClient) -> str:
def test_environment(registry_client: MLClient) -> str:

Copilot uses AI. Check for mistakes.


@pytest.fixture(scope="session")
def test_environment(registry_client: MLClient) -> str:
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The fixture accepts a 'registry_client' parameter but doesn't use it. Since the function just returns a hardcoded string, this parameter should be removed.

Suggested change
def test_environment(registry_client: MLClient) -> str:
def test_environment() -> str:

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +29


Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

There are three consecutive blank lines here. According to PEP 8, there should be at most two blank lines between top-level definitions.

Suggested change

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant