Claude finished @olivermeyer's task in 2m 10s —— View job

PR Review: Use GPUConfig in E2E tests ✅

I've completed a comprehensive review of this PR. The changes are well-designed and correct - they properly address the issue from PR #391 by using the GPUConfig class instead of manually constructing the metadata dict.

Summary

What changed: The _submit_and_validate() helper function now uses the GPUConfig Pydantic model to build GPU configuration metadata instead of manually constructing the dict. This ensures proper serialization behavior (excluding None values).

Why this matters: PR #391 added exclude_if=lambda v: v is None to the flex_start_max_run_duration_minutes field in GPUConfig. Tests that bypass the class and manually build dicts wouldn't benefit from this serialization logic, potentially causing issues when None values are included in the API payload.

Review Status: ✅ APPROVED - No blocking issues found

Critical Checks - All Passed ✅

1. Test Markers ✅

All tests in this file have proper @pytest.mark.e2e markers. The changed function _submit_and_validate() is a helper, not a test itself, so no marker needed.

# Verified all tests are properly marked
$ uv run pytest tests/aignostics/platform/e2e_test.py --collect-only
collected 7 items (all have e2e marker)

2. Code Quality ✅

• Ruff: All checks passed ✅
• MyPy: Pre-existing type annotation warnings (not introduced by this PR) ⚠️
• Logic: Correct usage of GPUConfig.model_dump() ✅

3. Commit Message ✅

Follows conventional commits: test: use GPUConfig in E2E test ✅

Technical Analysis

What the change does:

Before (manual dict construction):

custom_metadata = {
    "sdk": {
        "pipeline": {
            "gpu": {
                "gpu_type": PIPELINE_GPU_TYPE,
                "provisioning_mode": PIPELINE_GPU_PROVISIONING_MODE,
                "flex_start_max_run_duration_minutes": PIPELINE_GPU_FLEX_START_MAX_RUN_DURATION_MINUTES,  # ⚠️ Always included, even if None
                "max_gpus_per_slide": PIPELINE_MAX_GPUS_PER_SLIDE,
            },
            # ...
        }
    }
}

After (using GPUConfig):

gpu_config = GPUConfig(
    gpu_type=PIPELINE_GPU_TYPE,
    provisioning_mode=PIPELINE_GPU_PROVISIONING_MODE,
    max_gpus_per_slide=PIPELINE_MAX_GPUS_PER_SLIDE,
    flex_start_max_run_duration_minutes=PIPELINE_GPU_FLEX_START_MAX_RUN_DURATION_MINUTES,
)
custom_metadata = {
    "sdk": {
        "pipeline": {
            "gpu": gpu_config.model_dump(),  # ✅ Excludes None values via exclude_if
            # ...
        }
    }
}

Why this is correct:

1. Proper serialization: GPUConfig.model_dump() applies the exclude_if=lambda v: v is None rule from line 89 of _sdk_metadata.py
2. Test context: PIPELINE_GPU_FLEX_START_MAX_RUN_DURATION_MINUTES = None in both staging and production (from constants_test.py:52,97)
3. Provisioning mode: Tests use SPOT mode, not FLEX_START, so the field should be None and excluded
4. Validation: The GPUConfig.validate_flex_start_duration() validator ensures this is correct (raises error if field is set when mode != FLEX_START)

Observations & Recommendations

✅ Strengths

1. Minimal change scope: Author correctly chose not to refactor the entire custom_metadata to use RunSdkMetadata - keeping the PR focused
2. Proper class usage: Uses the Pydantic model as intended with .model_dump()
3. Consistent pattern: This is the same pattern used throughout the SDK (see platform/resources/runs.py)
4. Better maintainability: Changes to GPUConfig serialization logic will now automatically apply to these tests

⚠️ Pre-existing Issues (NOT introduced by this PR)

Type annotation warnings - These exist in the current codebase and are NOT caused by this PR:

tests/aignostics/platform/e2e_test.py:337: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
    def _submit_and_wait(  # noqa: PLR0913, PLR0917

Why this is acceptable:

• Tests are excluded from PyRight checks (pyrightconfig.json:8)
• MyPy strict mode warnings pre-exist
• Not a blocking issue per repository standards

If you want to fix (optional, separate PR):

def _submit_and_wait(
    application_id: str,
    application_version: str,
    payload: list[platform.InputItem],
    record_property: Any,  # pytest fixture, use Any
    due_date_seconds: int,
    deadline_seconds: int,
    timeout_seconds: int,
    tags: set[str] | None = None,
    checksum_attribute_key: str = "checksum_base64_crc32c",
) -> None:

Testing

The changes affect E2E test infrastructure. Tests marked with @pytest.mark.e2e, @pytest.mark.scheduled_only, and @pytest.mark.very_long_running will validate this in CI:

• ✅ test_platform_test_app_submit_and_wait - Uses updated code path
• ✅ test_platform_heta_app_submit - Uses updated code path
• ✅ test_platform_special_app_submit - Uses updated code path

Conclusion

Recommendation: ✅ APPROVE AND MERGE

This is a clean, focused fix that ensures E2E tests properly use the GPUConfig serialization logic introduced in PR #391. The implementation is correct, the scope is appropriate, and all critical checks pass.

What happens next:

1. CI will run E2E tests against staging environment
2. Tests will submit runs with properly serialized GPU config (excluding None values)
3. Platform API will receive clean metadata without null fields

No action required - the PR is ready to merge once CI passes.

Branch: test/set-flex-start-max-duration-only-not-none

Audit workflow failing because of vulnerabilities, will be addressed separately.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.
see 4 files with indirect coverage changes

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud