fix(job_attachment)!: Change osType to rootPathFormat (#37)

BREAKING CHANGES: - Renamed the field `osType` -> `rootPathFormat` - The workarounds that allowed the use of `source_os` in the path mapping rules are removed. Signed-off-by: Gahyun Suh <[email protected]> Signed-off-by: Graeme McHale <[email protected]>
aws-deadline · Feb 12, 2024 · 05ac613 · 05ac613
1 parent e5cb959
commit 05ac613
Show file tree

Hide file tree

Showing 9 changed files with 24 additions and 99 deletions.
diff --git a/pyproject.toml b/pyproject.toml
@@ -8,7 +8,7 @@ dynamic = ["version"]
 dependencies = [
     "requests ~= 2.29",
     "boto3 ~= 1.26",
-    "deadline == 0.25.*",
+    "deadline == 0.28.*",
     "openjd-sessions == 0.2.*",
     # tomli became tomllib in standard library in Python 3.11
     "tomli >= 1.1.0 ; python_version<'3.11'",

diff --git a/src/deadline_worker_agent/api_models.py b/src/deadline_worker_agent/api_models.py
@@ -206,8 +206,8 @@ class ManifestProperties(TypedDict):
     fileSystemLocationName: NotRequired[str]
     """The name of the file system location"""
 
-    osType: str
-    """The operating system family (linux/windows/macos) associated with the asset's rootPath"""
+    rootPathFormat: str
+    """The operating system family (posix/windows) associated with the asset's rootPath"""
 
     inputManifestPath: NotRequired[str]
     """A (partial) key path of an S3 object that points to a file manifest.
@@ -222,11 +222,7 @@ class ManifestProperties(TypedDict):
 
 
 class PathMappingRule(TypedDict):
-    # TODO: remove sourceOs and make sourcePathFormat required
-    sourceOs: NotRequired[str]
-    """The operating system family (posix/windows) associated with the source path"""
-
-    sourcePathFormat: NotRequired[str]
+    sourcePathFormat: str
     """The path format associated with the source path (windows vs posix)"""
 
     sourcePath: str

diff --git a/src/deadline_worker_agent/boto/shim.py b/src/deadline_worker_agent/boto/shim.py
@@ -261,12 +261,6 @@ def batch_get_job_entity(
                                 }
                             },
                             "pathMappingRules": [
-                                {
-                                    # TODO: remove once sourceOs is removed from response
-                                    "sourceOs": "windows",
-                                    "sourcePath": "C:/windows/path",
-                                    "destinationPath": "/linux/path",
-                                },
                                 {
                                     "sourcePathFormat": "windows",
                                     "sourcePath": "C:/windows/path",

diff --git a/src/deadline_worker_agent/sessions/job_entities/job_attachment_details.py b/src/deadline_worker_agent/sessions/job_entities/job_attachment_details.py
@@ -54,8 +54,8 @@ class JobAttachmentManifestProperties:
     root_path: str
     """The input root path to be mapped"""
 
-    os_type: str
-    """The operating system family (linux/windows/macos) associated with the asset's root_path"""
+    root_path_format: str
+    """The operating system family (posix/windows) associated with the asset's root_path"""
 
     file_system_location_name: str | None = None
     """The name of the file system location"""
@@ -122,7 +122,7 @@ def from_boto(
                     input_manifest_path=manifest_properties.get("inputManifestPath", ""),
                     input_manifest_hash=manifest_properties.get("inputManifestHash", ""),
                     root_path=manifest_properties["rootPath"],
-                    os_type=manifest_properties["osType"],
+                    root_path_format=manifest_properties["rootPathFormat"],
                 )
                 for manifest_properties in job_attachments_details_data["attachments"]["manifests"]
             ],
@@ -175,7 +175,7 @@ def validate_entity_data(cls, entity_data: dict[str, Any]) -> JobAttachmentDetai
                                     required=False,
                                 ),
                                 Field(key="rootPath", expected_type=str, required=True),
-                                Field(key="osType", expected_type=str, required=True),
+                                Field(key="rootPathFormat", expected_type=str, required=True),
                                 Field(
                                     key="outputRelativeDirectories",
                                     expected_type=list,

diff --git a/src/deadline_worker_agent/sessions/job_entities/job_details.py b/src/deadline_worker_agent/sessions/job_entities/job_details.py
@@ -64,12 +64,7 @@ def path_mapping_api_model_to_openjd(
     to the format expected by Open Job Description. effectively camelCase to snake_case"""
     rules: list[OPENJDPathMappingRule] = []
     for api_rule in path_mapping_rules:
-        api_source_path_format = (
-            # delete sourceOs once removed from api response
-            api_rule["sourcePathFormat"]
-            if "sourcePathFormat" in api_rule
-            else api_rule["sourceOs"]
-        )
+        api_source_path_format = api_rule["sourcePathFormat"]
         source_path_format: PathFormat = (
             PathFormat.WINDOWS if api_source_path_format.lower() == "windows" else PathFormat.POSIX
         )
@@ -304,9 +299,7 @@ def validate_entity_data(cls, entity_data: dict[str, Any]) -> JobDetailsData:
                 validate_object(
                     data=path_mapping_rule,
                     fields=(
-                        # TODO: remove sourceOs and make sourcePathFormat required
-                        Field(key="sourceOs", expected_type=str, required=False),
-                        Field(key="sourcePathFormat", expected_type=str, required=False),
+                        Field(key="sourcePathFormat", expected_type=str, required=True),
                         Field(key="sourcePath", expected_type=str, required=True),
                         Field(key="destinationPath", expected_type=str, required=True),
                     ),

diff --git a/src/deadline_worker_agent/sessions/session.py b/src/deadline_worker_agent/sessions/session.py
@@ -4,7 +4,7 @@
 
 import os
 from concurrent.futures import ThreadPoolExecutor
-from dataclasses import dataclass, fields
+from dataclasses import dataclass
 from datetime import datetime, timedelta, timezone
 from functools import partial
 from logging import getLogger, LoggerAdapter
@@ -55,7 +55,7 @@
     PosixFileSystemPermissionSettings,
     JobAttachmentS3Settings,
     ManifestProperties,
-    OperatingSystemFamily,
+    PathFormat,
 )
 from deadline.job_attachments.progress_tracker import ProgressReportMetadata
 
@@ -789,7 +789,7 @@ def progress_handler(job_upload_status: ProgressReportMetadata) -> bool:
                     ManifestProperties(
                         rootPath=manifest_properties.root_path,
                         fileSystemLocationName=manifest_properties.file_system_location_name,
-                        osType=OperatingSystemFamily(manifest_properties.os_type),
+                        rootPathFormat=PathFormat(manifest_properties.root_path_format),
                         inputManifestPath=manifest_properties.input_manifest_path,
                         inputManifestHash=manifest_properties.input_manifest_hash,
                         outputRelativeDirectories=manifest_properties.output_relative_directories,
@@ -840,22 +840,6 @@ def progress_handler(job_upload_status: ProgressReportMetadata) -> bool:
             f"Summary Statistics for file downloads:\n{download_summary_statistics}"
         )
 
-        # TODO: remove path_mapping_rule workaround once deadline-cloud and openjd are both upgraded
-        source_path_format_key = "source_path_format"
-        source_os_key = "source_os"
-        use_source_path_format = any(
-            f.name == source_path_format_key for f in fields(PathMappingRule)
-        )
-        for rule in path_mapping_rules:
-            if use_source_path_format:
-                if source_os_key in rule:
-                    rule[source_path_format_key] = rule[source_os_key]
-                    del rule[source_os_key]
-            else:
-                if source_path_format_key in rule:
-                    rule[source_os_key] = rule[source_path_format_key]
-                    del rule[source_path_format_key]
-
         job_attachment_path_mappings = [
             PathMappingRule.from_dict(rule) for rule in path_mapping_rules
         ]
@@ -1053,12 +1037,12 @@ def _sync_asset_outputs(
         for manifest_properties in job_attachment_details.manifests:
             manifest_properties_list.append(
                 ManifestProperties(
-                    manifest_properties.root_path,
-                    manifest_properties.file_system_location_name,
-                    OperatingSystemFamily(manifest_properties.os_type),
-                    manifest_properties.input_manifest_path,
-                    manifest_properties.input_manifest_hash,
-                    manifest_properties.output_relative_directories,
+                    rootPath=manifest_properties.root_path,
+                    fileSystemLocationName=manifest_properties.file_system_location_name,
+                    rootPathFormat=PathFormat(manifest_properties.root_path_format),
+                    inputManifestPath=manifest_properties.input_manifest_path,
+                    inputManifestHash=manifest_properties.input_manifest_hash,
+                    outputRelativeDirectories=manifest_properties.output_relative_directories,
                 )
             )
 

diff --git a/test/unit/conftest.py b/test/unit/conftest.py
@@ -13,7 +13,7 @@
     AssetLoadingMethod,
     Attachments,
     ManifestProperties,
-    OperatingSystemFamily,
+    PathFormat,
 )
 from openjd.model import SchemaVersion
 from openjd.sessions import (
@@ -144,7 +144,7 @@ def action_id() -> str:
             manifests=[
                 ManifestProperties(
                     rootPath="/tmp",
-                    osType=OperatingSystemFamily.LINUX,
+                    rootPathFormat=PathFormat.POSIX,
                     inputManifestPath="rootPrefix/Manifests/farm-1/queue-1/Inputs/0000/0123_input.xxh128",
                     inputManifestHash="inputmanifesthash",
                     outputRelativeDirectories=["test_outputs"],
@@ -206,7 +206,7 @@ def job_attachment_manifest_properties(
 ) -> JobAttachmentManifestProperties:
     return JobAttachmentManifestProperties(
         root_path="/foo/bar",
-        os_type="linux",
+        root_path_format="posix",
         file_system_location_name="",
         input_manifest_path=f"{queue_job_attachment_settings.root_prefix}/Manifests/{farm_id}/{queue_id}/Inputs/0000/0123_input.xxh128",
         input_manifest_hash="inputmanifesthash",

diff --git a/test/unit/sessions/test_job_entities.py b/test/unit/sessions/test_job_entities.py
@@ -115,8 +115,7 @@ class TestJobEntity:
             pytest.param(
                 [
                     {
-                        # TODO: swap to sourcePathFormat once sourceOs removed
-                        "sourceOs": "windows",
+                        "sourcePathFormat": "windows",
                         "sourcePath": "Z:/artist/windows/path",
                         "destinationPath": "/mnt/worker/windows/path",
                     },
@@ -488,7 +487,7 @@ def test_cache_entities(
                 "manifests": [
                     {
                         "rootPath": "/mnt/share",
-                        "osType": "linux",
+                        "rootPathFormat": "posix",
                         "outputRelativeDirectories": ["output"],
                     }
                 ]

diff --git a/test/unit/sessions/test_session.py b/test/unit/sessions/test_session.py
@@ -586,47 +586,6 @@ def test_sync_asset_inputs(
             else:
                 session.sync_asset_inputs(cancel=cancel, **args)  # type: ignore[arg-type]
 
-    def test_job_attachments_path_mapping_rules_compatibility(
-        self,
-        session: Session,
-        mock_asset_sync: MagicMock,
-    ):
-        """
-        Tests that the path mapping rules received from job_attachments's sync_inputs
-        can use both the older 'source_os' and the newer 'source_path_format' based
-        on whatever fields are required in openjd's PathMappingRule
-
-        Remove this test once both openjd and job_attachments both use source_path_format
-        """
-        # GIVEN
-        mock_sync_inputs: MagicMock = mock_asset_sync.sync_inputs
-        path_mapping_rules = [
-            {
-                # TODO: remove sourceOs once removed
-                "source_os": "windows",
-                "source_path": "Z:/artist/windows/path",
-                "destination_path": "/mnt/worker/windows/path",
-            },
-            {
-                "source_path_format": "posix",
-                "source_path": "/artist/linux",
-                "destination_path": "/mnt/worker/linux",
-            },
-        ]
-        mock_sync_inputs.return_value = ({}, path_mapping_rules)
-        cancel = Event()
-
-        sync_asset_inputs_args = {
-            "job_attachment_details": JobAttachmentDetails(
-                manifests=[],
-                asset_loading_method=AssetLoadingMethod.PRELOAD,
-            )
-        }
-
-        # WHEN / THEN
-        session.sync_asset_inputs(cancel=cancel, **sync_asset_inputs_args)  # type: ignore[arg-type]
-        # No errors on generating path mapping rules - success!
-
 
 class TestSessionInnerRun:
     """Test cases for Session._run()"""