feat(asset-cli): asset snapshot subcommand to capture manifests #369

stangch · 2024-06-24T23:40:03Z

What was the problem/requirement? (What/Why)

Allow users to call CLI command to capture directories of assets into manifests.

What was the solution? (How)

Added deadline asset snapshot subcommand to allow capture of manifests. Includes:

capturing manifest file into specified directory of assets
recursive capture of directories
can specify destination of where manifest can be created

What is the impact of this change?

Exposes functionality of creating manifests

How was this change tested?

Added unit tests mocking snapshot functionality at test/unit/deadline_client/cli/test_cli_asset.py, passing successfully
Added integ tests with temporary files in filesystem at test/integ/cli/test_cli_asset.py, passing successfully
Manual end to end tests with sample directories with assets:

deadline-cloud-stangch % hatch run deadline asset snapshot --root-dir /Users/stangch/Desktop/test_asset -r
Manifest creation path defaulted to /Users/stangch/Desktop/test_asset 

Hashing Attachments  [####################################]  100%
Hashing Summary:
    Processed 5 files totaling 7.5 KB.
    Skipped re-processing 1 files totaling 37.0 B.
    Total processing time of 0.00932 seconds at 804.07 KB/s.

Manifest created at /Users/stangch/Desktop/test_asset

Was this change documented?

N/A

Is this a breaking change?

N/A

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Tang <[email protected]>

src/deadline/client/api/_submit_job_bundle.py

src/deadline/client/cli/_common.py

src/deadline/client/cli/_groups/asset_group.py

test/unit/deadline_client/cli/test_cli_asset.py

src/deadline/client/api/_submit_job_bundle.py

src/deadline/client/cli/_groups/asset_group.py

godobyte · 2024-06-27T21:04:44Z

src/deadline/client/cli/_groups/asset_group.py

+    hash_callback_manager = _ProgressBarCallbackManager(length=100, label="Hashing Attachments")
+
+    upload_group = asset_manager.prepare_paths_for_upload(
+        input_paths=inputs, output_paths=[root_dir], referenced_paths=[]


Curious - what is this referenced_paths?

There's some comments on this page: https://github.com/ddneilson/deadline-cloud-samples/blob/the_job_bundle/job_bundles/README.md

tldr: it's neither input nor output, but in case job references the name of the folder; in the case of snapshot + upload, the main goal is to upload files we care about, so I don't think referenced_paths are relevant here.

godobyte · 2024-06-27T21:12:14Z

src/deadline/job_attachments/upload.py

@@ -182,7 +179,7 @@ def upload_assets(

        if manifest_write_dir:
            self._write_local_manifest(
-                manifest_write_dir, manifest_name, full_manifest_key, manifest
+                manifest_write_dir, manifest_name, full_manifest_key, manifest, None


What about adding default to the function parameter rather than passing None from the caller?

godobyte · 2024-06-27T21:19:08Z

test/integ/cli/test_cli_asset.py

+        with tempfile.TemporaryDirectory() as tmpdirname:
+            yield tmpdirname


Nit - the naming is a bit confusing, I assume it's tmpdir rather than name?

godobyte · 2024-06-27T21:22:05Z

test/integ/cli/test_cli_asset.py

+            manifest_data = json.load(f)
+            assert manifest_data["paths"][0]["path"] == "file.txt"
+            assert (
+                manifest_data["paths"][0]["hash"] == "0741993e50c8bc250cefba3959c81eb8"


How did you get this hash, could it be a call with the input? i.e. we don't need to update the hash if there is any change to the input.

godobyte · 2024-06-27T21:28:54Z

test/integ/cli/test_cli_asset.py

+                manifest_data["paths"][0]["hash"] == "a5fc4a07191e2c90364319d2fd503cc1"
+            )  # hashed with xxh128
+            assert manifest_data["paths"][1]["path"] == posixpath.join(
+                "subdir1", "subdir2", "subdir_file.txt"


In general for tests, the test input can be defined as a variable and pass to the call and assertion. Take a look at how it's done in other test cli implementation.

jericht · 2024-06-27T19:58:01Z

src/deadline/client/cli/_groups/asset_group.py

@@ -33,11 +42,61 @@ def cli_asset():
    default=False,
 )
 @_handle_error
-def asset_snapshot(**args):
+def asset_snapshot(root_dir: str, manifest_out: str, recursive: bool, **args):


Question for the type annotation: Is manifest_out always a str? Is it a different type when the user does not provide --manifest-out?

I think the click library doesn't allow users to specify optional in the function params, rather in the above @click.options, so I added a default=None for --manifest-out

Right, so the typing should say it can either be None or str right?

godobyte · 2024-06-28T19:18:49Z

This CLI implementation seems to capture the basic functionality. Were you able to call it and verify? If so, could you add to testing evidence?

test/integ/cli/test_cli_asset.py

test/unit/deadline_client/cli/test_cli_asset.py

test/integ/cli/test_cli_asset.py

test/unit/deadline_client/cli/test_cli_asset.py

jericht

There's still some comments I have on the tests that I think we should address, but we can do that in a future PR. Approved.

jericht · 2024-07-03T02:44:25Z

test/unit/deadline_client/cli/test_cli_asset.py

+        root_dir = str(tmp_path)
+
+        temp_file = tmp_path / "temp_file.txt"
+        temp_file.touch()


nit: Ideally we shouldn't be creating files/dirs in our unit tests, but we can address this later.

jericht · 2024-07-03T02:46:13Z

test/unit/deadline_client/cli/test_cli_asset.py

+    def test_valid_manifest_out(
+        self, tmp_path, mock_prepare_paths_for_upload, mock_hash_attachments, upload_group_mock
+    ):
+        """
+        Tests if CLI snapshot command correctly calls with --manifest-out arguement
+        """
+        root_dir = str(tmp_path)
+        manifest_out_dir = tmp_path / "manifest_out"
+        manifest_out_dir.mkdir()
+
+        temp_file = tmp_path / "temp_file.txt"
+        temp_file.touch()
+
+        mock_prepare_paths_for_upload.return_value = upload_group_mock
+
+        runner = CliRunner()
+        result = runner.invoke(
+            main,
+            [
+                "asset",
+                "snapshot",
+                "--root-dir",
+                root_dir,
+                "--manifest-out",
+                str(manifest_out_dir),
+            ],
+        )
+
+        assert result.exit_code == 0
+        mock_prepare_paths_for_upload.assert_called_once_with(
+            input_paths=[str(temp_file)], output_paths=[root_dir], referenced_paths=[]
+        )
+        mock_hash_attachments.assert_called_once()


Looks like we're missing an assertion here to verify the command would have actually written the manifest file to manifest_out_dir

jericht · 2024-07-03T02:47:43Z

test/unit/deadline_client/cli/test_cli_asset.py

+        runner = CliRunner()
+        result = runner.invoke(main, ["asset", "snapshot", "--root-dir", invalid_root_dir])
+
+        assert result.exit_code != 0


nit: if we know what the expected exit code should be, we should assert equality on that expected value rather than checking for non-zero exit code.

test/integ/cli/test_cli_asset.py

Signed-off-by: Tang <[email protected]>

stangch added 7 commits June 18, 2024 10:01

feat(asset-cli): added asset subcommand template and dummy commands

469efd6

Signed-off-by: Tang <[email protected]>

feat(asset-cli): snaphsot command POC

fac5486

Signed-off-by: Tang <[email protected]>

feat(asset-cli): added asset subcommand template and dummy commands

65f73dc

Signed-off-by: Tang <[email protected]>

pulled changes from mainline branch

43f4ddc

Signed-off-by: Tang <[email protected]>

feat(asset-cli): added asset subcommand template and dummy commands

42a6d16

Signed-off-by: Tang <[email protected]>

pulled changes from mainline branch

dbac70e

Signed-off-by: Tang <[email protected]>

feat(asset-cli): added snapshot subcommmand for deadline asset

1bbd42d

Signed-off-by: Tang <[email protected]>

stangch requested a review from a team as a code owner June 24, 2024 23:40

stangch force-pushed the snapshot_prod branch 9 times, most recently from eb1ad9a to 3bf6fa6 Compare June 25, 2024 01:08

jericht self-requested a review June 25, 2024 01:22

godobyte self-assigned this Jun 25, 2024

stangch changed the base branch from mainline to feature_assets_cli June 25, 2024 20:15

feat(asset-cli): added snapshot subcommmand for deadline asset

e67bf2c

Signed-off-by: Tang <[email protected]>

stangch force-pushed the snapshot_prod branch from e41835a to e67bf2c Compare June 25, 2024 21:26

stangch added 3 commits June 25, 2024 14:28

Merge branch 'feature_assets_cli' into snapshot_prod

ae6cbf1

merge from feature to fork

6168cfd

merge conflicting branches

e75105e

stangch force-pushed the snapshot_prod branch 3 times, most recently from f299b41 to e31c462 Compare June 25, 2024 21:44

jericht requested changes Jun 26, 2024

View reviewed changes

stangch force-pushed the snapshot_prod branch from e31c462 to bda7958 Compare June 27, 2024 03:54

stangch force-pushed the snapshot_prod branch 6 times, most recently from fbeb44e to 5daab26 Compare June 27, 2024 20:28

godobyte reviewed Jun 27, 2024

View reviewed changes

jericht reviewed Jun 28, 2024

View reviewed changes

stangch force-pushed the snapshot_prod branch from 5daab26 to 1be6097 Compare June 28, 2024 18:26

godobyte approved these changes Jun 28, 2024

View reviewed changes

jericht reviewed Jun 29, 2024

View reviewed changes

stangch force-pushed the snapshot_prod branch 3 times, most recently from 40716c2 to eaebbe1 Compare July 2, 2024 22:40

jericht approved these changes Jul 3, 2024

View reviewed changes

stangch force-pushed the snapshot_prod branch from eaebbe1 to 809d8dd Compare July 3, 2024 17:35

jericht requested review from godobyte and jericht July 3, 2024 23:11

jericht approved these changes Jul 3, 2024

View reviewed changes

merge conflicting branches

10da6d9

Signed-off-by: Tang <[email protected]>

stangch force-pushed the snapshot_prod branch from 809d8dd to 10da6d9 Compare July 4, 2024 17:41

stangch merged commit a954d40 into aws-deadline:feature_assets_cli Jul 4, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(asset-cli): asset snapshot subcommand to capture manifests #369

feat(asset-cli): asset snapshot subcommand to capture manifests #369

stangch commented Jun 24, 2024 •

edited

Loading

godobyte Jun 27, 2024

stangch Jun 27, 2024

godobyte Jun 27, 2024

godobyte Jun 27, 2024

godobyte Jun 27, 2024

godobyte Jun 27, 2024

jericht Jun 27, 2024

stangch Jun 28, 2024

jericht Jun 29, 2024

godobyte commented Jun 28, 2024

jericht left a comment

jericht Jul 3, 2024

jericht Jul 3, 2024

jericht Jul 3, 2024

		with tempfile.TemporaryDirectory() as tmpdirname:
		yield tmpdirname

feat(asset-cli): asset snapshot subcommand to capture manifests #369

feat(asset-cli): asset snapshot subcommand to capture manifests #369

Conversation

stangch commented Jun 24, 2024 • edited Loading

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godobyte commented Jun 28, 2024

jericht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stangch commented Jun 24, 2024 •

edited

Loading