Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(job_attachment)!: Change osType and source_os names #37

Merged
merged 1 commit into from
Sep 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Expand Down
10 changes: 3 additions & 7 deletions src/deadline_worker_agent/api_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
6 changes: 0 additions & 6 deletions src/deadline_worker_agent/boto/shim.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"""
Expand Down Expand Up @@ -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"]
],
Expand Down Expand Up @@ -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,
Expand Down
11 changes: 2 additions & 9 deletions src/deadline_worker_agent/sessions/job_entities/job_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -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),
),
Expand Down
34 changes: 9 additions & 25 deletions src/deadline_worker_agent/sessions/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -55,7 +55,7 @@
PosixFileSystemPermissionSettings,
JobAttachmentS3Settings,
ManifestProperties,
OperatingSystemFamily,
PathFormat,
)
from deadline.job_attachments.progress_tracker import ProgressReportMetadata

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
]
Expand Down Expand Up @@ -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,
)
)

Expand Down
6 changes: 3 additions & 3 deletions test/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
AssetLoadingMethod,
Attachments,
ManifestProperties,
OperatingSystemFamily,
PathFormat,
)
from openjd.model import SchemaVersion
from openjd.sessions import (
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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",
Expand Down
5 changes: 2 additions & 3 deletions test/unit/sessions/test_job_entities.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
Expand Down Expand Up @@ -488,7 +487,7 @@ def test_cache_entities(
"manifests": [
{
"rootPath": "/mnt/share",
"osType": "linux",
"rootPathFormat": "posix",
"outputRelativeDirectories": ["output"],
}
]
Expand Down
41 changes: 0 additions & 41 deletions test/unit/sessions/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()"""
Expand Down