perf(python): symlink layer dependencies instead of copying them

aws_lambda_builders/actions.py

@@ -137,8 +137,15 @@ def execute(self):
         for source_file in source_files:
             source_path = Path(self._source_dir, source_file)
             destination_path = Path(self._dest_dir, source_file)
-            if destination_path.exists():
+            if destination_path.is_symlink() or destination_path.is_file():
+                # is_symlink() is checked first and deliberately: a dangling symlink is not
+                # exists(), so the previous exists() check left it in place and os.symlink then
+                # failed with FileExistsError.
                 os.remove(destination_path)
+            elif destination_path.is_dir():
+                # A real directory left behind by an earlier copying build. os.remove cannot remove
+                # it, and leaving it would shadow the symlink we are about to create.
+                shutil.rmtree(destination_path)
             else:
                 os.makedirs(destination_path.parent, exist_ok=True)
             utils.create_symlink_or_copy(str(source_path), str(destination_path))

aws_lambda_builders/workflows/python_pip/workflow.py

@@ -115,8 +115,15 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
         # folder
         if self.dependencies_dir and self.combine_dependencies:
             # when copying downloaded dependencies back to artifacts folder, don't exclude anything
-            # symlinking python dependencies is disabled for now since it is breaking sam local commands
-            if False and is_experimental_build_improvements_enabled(self.experimental_flags):
+            #
+            # Symlinking is only safe for layers. A layer's artifacts are packed into a tarball
+            # (which dereferences symlinks) before they reach the local invoke container, whereas a
+            # function's artifacts are bind-mounted at /var/task, where a symlink pointing outside
+            # the mount dangles unless the caller passes `sam local invoke --mount-symlinks`. That
+            # option does not exist on `sam local start-api` / `start-lambda`, so linking function
+            # dependencies would break those commands -- which is why this was disabled wholesale in
+            # https://github.com/aws/aws-lambda-builders/pull/391. Keep copying for functions.
+            if self.is_building_layer and is_experimental_build_improvements_enabled(self.experimental_flags):
                 self._actions.append(LinkSourceAction(self.dependencies_dir, artifacts_dir))
             else:
                 self._actions.append(CopySourceAction(self.dependencies_dir, artifacts_dir))

tests/unit/test_actions.py

@@ -1,3 +1,5 @@
+import os
+import tempfile
 from pathlib import Path
 from unittest import TestCase
 from unittest.mock import ANY, patch
@@ -13,6 +15,7 @@
     CleanUpAction,
     DependencyManager,
     LinkSinglePathAction,
+    LinkSourceAction,
 )
 
 
@@ -265,6 +268,57 @@ def _convert_strings_to_paths(source_dest_list):
         return map(lambda item: (Path(item[0]), Path(item[1])), source_dest_list)
 
 
+class TestLinkSourceAction(TestCase):
+    def setUp(self):
+        self._tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self._tmp.cleanup)
+        self.source_dir = Path(self._tmp.name, "deps")
+        self.dest_dir = Path(self._tmp.name, "artifacts")
+        (self.source_dir / "somepkg").mkdir(parents=True)
+        (self.source_dir / "somepkg" / "__init__.py").write_text("hello")
+        (self.source_dir / "six.py").write_text("six")
+        self.dest_dir.mkdir()
+
+    def _execute(self):
+        LinkSourceAction(str(self.source_dir), str(self.dest_dir)).execute()
+
+    def _assert_linked(self):
+        for name in ("somepkg", "six.py"):
+            destination = self.dest_dir / name
+            self.assertTrue(destination.is_symlink(), f"{name} should be a symlink")
+            self.assertEqual(os.path.realpath(destination), str((self.source_dir / name).resolve()))
+        self.assertEqual((self.dest_dir / "somepkg" / "__init__.py").read_text(), "hello")
+
+    def test_links_into_an_empty_destination(self):
+        self._execute()
+        self._assert_linked()
+
+    def test_replaces_a_real_directory_left_by_an_earlier_copying_build(self):
+        # A build that copied dependencies leaves real directories behind. Without --clean they
+        # survive into the next build, and os.remove() cannot remove a directory.
+        stale = self.dest_dir / "somepkg"
+        stale.mkdir()
+        (stale / "__init__.py").write_text("stale")
+        (self.dest_dir / "six.py").write_text("stale")
+
+        self._execute()
+        self._assert_linked()
+
+    def test_replaces_a_dangling_symlink(self):
+        # A dangling symlink is not exists(), so it used to be left in place and os.symlink then
+        # raised FileExistsError, which create_symlink_or_copy swallowed into a full copy.
+        (self.dest_dir / "six.py").symlink_to(self._tmp.name + "/gone")
+        self.assertFalse((self.dest_dir / "six.py").exists())
+
+        self._execute()
+        self._assert_linked()
+
+    def test_is_idempotent(self):
+        self._execute()
+        self._execute()
+        self._assert_linked()
+
+
 class TestLinkSinglePathAction(TestCase):
     @patch("aws_lambda_builders.actions.os.makedirs")
     @patch("aws_lambda_builders.utils.create_symlink_or_copy")

tests/unit/workflows/python_pip/test_workflow.py

@@ -12,14 +12,24 @@
 
 
 @parameterized_class(
-    ("experimental_flags",),
+    ("experimental_flags", "is_building_layer"),
     [
-        ([]),
-        ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE]),
+        ([], False),
+        ([], True),
+        ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], False),
+        ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], True),
     ],
 )
 class TestPythonPipWorkflow(TestCase):
     experimental_flags = []
+    is_building_layer = False
+
+    @property
+    def expects_linked_dependencies(self):
+        """Dependencies are symlinked only when the build performance flag is on AND we are
+        building a layer -- a function's artifacts are bind-mounted into the local invoke
+        container, where symlinks pointing outside the mount dangle."""
+        return bool(self.experimental_flags) and self.is_building_layer
 
     def setUp(self):
         self.osutils = OSUtils()
@@ -121,10 +131,10 @@ def test_workflow_sets_up_actions_without_download_dependencies_with_dependencie
             dependencies_dir="dep",
             download_dependencies=False,
             experimental_flags=self.experimental_flags,
+            is_building_layer=self.is_building_layer,
         )
         self.assertEqual(len(self.workflow.actions), 2)
-        # symlinking python dependencies is disabled for now since it is breaking sam local commands
-        if False and self.experimental_flags:
+        if self.expects_linked_dependencies:
             self.assertIsInstance(self.workflow.actions[0], LinkSourceAction)
         else:
             self.assertIsInstance(self.workflow.actions[0], CopySourceAction)
@@ -143,12 +153,12 @@ def test_workflow_sets_up_actions_with_download_dependencies_and_dependencies_di
             dependencies_dir="dep",
             download_dependencies=True,
             experimental_flags=self.experimental_flags,
+            is_building_layer=self.is_building_layer,
         )
         self.assertEqual(len(self.workflow.actions), 4)
         self.assertIsInstance(self.workflow.actions[0], CleanUpAction)
         self.assertIsInstance(self.workflow.actions[1], PythonPipBuildAction)
-        # symlinking python dependencies is disabled for now since it is breaking sam local commands
-        if False and self.experimental_flags:
+        if self.expects_linked_dependencies:
             self.assertIsInstance(self.workflow.actions[2], LinkSourceAction)
         else:
             self.assertIsInstance(self.workflow.actions[2], CopySourceAction)
@@ -193,6 +203,30 @@ def test_workflow_sets_up_actions_without_combine_dependencies(self):
         self.assertIsInstance(self.workflow.actions[1], PythonPipBuildAction)
         self.assertIsInstance(self.workflow.actions[2], CopySourceAction)
 
+    def test_layer_links_dependencies_while_function_copies_them(self):
+        """The layer/function distinction is the whole reason linking is safe at all, so assert the
+        contrast on inputs that are otherwise identical -- otherwise a future change that drops
+        is_building_layer still passes every other test in this class."""
+
+        def actions_for(is_building_layer):
+            osutils_mock = Mock(spec=self.osutils)
+            osutils_mock.file_exists.return_value = True
+            return PythonPipWorkflow(
+                "source",
+                "artifacts",
+                "scratch_dir",
+                "manifest",
+                runtime="python3.9",
+                osutils=osutils_mock,
+                dependencies_dir="dep",
+                download_dependencies=True,
+                experimental_flags=[EXPERIMENTAL_FLAG_BUILD_PERFORMANCE],
+                is_building_layer=is_building_layer,
+            ).actions
+
+        self.assertIsInstance(actions_for(is_building_layer=True)[2], LinkSourceAction)
+        self.assertIsInstance(actions_for(is_building_layer=False)[2], CopySourceAction)
+
     @patch("aws_lambda_builders.workflows.python_pip.workflow.PythonPipBuildAction")
     def test_must_build_with_architecture(self, PythonPipBuildActionMock):
         self.workflow = PythonPipWorkflow(