From 0bb1c80457c86886bc485633fd2191b8e16642a4 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Thu, 31 Oct 2024 18:58:12 -0700 Subject: [PATCH 01/12] fix container task local execution path key extraction Signed-off-by: wayner0628 --- flytekit/core/container_task.py | 22 +++++++++++++++++-- .../unit/core/test_local_raw_container.py | 4 ++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/flytekit/core/container_task.py b/flytekit/core/container_task.py index b2efda772e..08eeadd0e0 100644 --- a/flytekit/core/container_task.py +++ b/flytekit/core/container_task.py @@ -112,22 +112,40 @@ def _extract_command_key(self, cmd: str, **kwargs) -> Any: return match.group(1) return None + def _extract_path_command_key(self, cmd: str, input_data_dir: str) -> Optional[str]: + """ + Extract the key from the path-like command using regex. + """ + import re + + input_regex = fr"{re.escape(input_data_dir)}/(.+)$" + match = re.match(input_regex, cmd) + if match: + return match.group(1) + return None + def _render_command_and_volume_binding(self, cmd: str, **kwargs) -> Tuple[str, Dict[str, Dict[str, str]]]: """ We support template-style references to inputs, e.g., "{{.inputs.infile}}". + + For FlyteFile and FlyteDirectory commands, e.g., "/var/inputs/inputs", we extract the key from strings that begin with the specified + `input_data_dir`. """ from flytekit.types.directory import FlyteDirectory from flytekit.types.file import FlyteFile command = "" volume_binding = {} - k = self._extract_command_key(cmd) + path_k = self._extract_path_command_key(cmd, self._input_data_dir) + k = path_k if path_k else self._extract_command_key(cmd) if k: input_val = kwargs.get(k) if type(input_val) in [FlyteFile, FlyteDirectory]: + if not path_k: + raise AssertionError("FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}") local_flyte_file_or_dir_path = str(input_val) - remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k.replace(".", "/")) # type: ignore + remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k) volume_binding[local_flyte_file_or_dir_path] = { "bind": remote_flyte_file_or_dir_path, "mode": "rw", diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index c92d2b66be..ad42ad3c5c 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -33,7 +33,7 @@ def test_flytefile_wf(): command=[ "python", "write_flytefile.py", - "{{.inputs.inputs}}", + "/var/inputs/inputs", "/var/outputs/out", ], ) @@ -80,7 +80,7 @@ def test_flytedir_wf(): command=[ "python", "write_flytedir.py", - "{{.inputs.inputs}}", + "/var/inputs/inputs", "/var/outputs/out", ], ) From 938be376b196e4d2d18d32f187fcd8b492372541 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Thu, 31 Oct 2024 19:22:08 -0700 Subject: [PATCH 02/12] lint Signed-off-by: wayner0628 --- flytekit/core/container_task.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flytekit/core/container_task.py b/flytekit/core/container_task.py index 08eeadd0e0..2bbef3e436 100644 --- a/flytekit/core/container_task.py +++ b/flytekit/core/container_task.py @@ -112,7 +112,7 @@ def _extract_command_key(self, cmd: str, **kwargs) -> Any: return match.group(1) return None - def _extract_path_command_key(self, cmd: str, input_data_dir: str) -> Optional[str]: + def _extract_path_command_key(self, cmd: str, input_data_dir: Optional[str]) -> Optional[str]: """ Extract the key from the path-like command using regex. """ @@ -145,7 +145,7 @@ def _render_command_and_volume_binding(self, cmd: str, **kwargs) -> Tuple[str, D if not path_k: raise AssertionError("FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}") local_flyte_file_or_dir_path = str(input_val) - remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k) + remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k) # type: ignore volume_binding[local_flyte_file_or_dir_path] = { "bind": remote_flyte_file_or_dir_path, "mode": "rw", From b516bf130be728c70f2a00a2f226d217ba770cb4 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Thu, 31 Oct 2024 19:27:05 -0700 Subject: [PATCH 03/12] lint Signed-off-by: wayner0628 --- flytekit/core/container_task.py | 1 + 1 file changed, 1 insertion(+) diff --git a/flytekit/core/container_task.py b/flytekit/core/container_task.py index 2bbef3e436..826c741c3f 100644 --- a/flytekit/core/container_task.py +++ b/flytekit/core/container_task.py @@ -118,6 +118,7 @@ def _extract_path_command_key(self, cmd: str, input_data_dir: Optional[str]) -> """ import re + input_data_dir = input_data_dir or "" input_regex = fr"{re.escape(input_data_dir)}/(.+)$" match = re.match(input_regex, cmd) if match: From ba94edf3c725ef157550c648320aa599106ea4cd Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Thu, 31 Oct 2024 19:30:36 -0700 Subject: [PATCH 04/12] lint Signed-off-by: wayner0628 --- flytekit/core/container_task.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/flytekit/core/container_task.py b/flytekit/core/container_task.py index 826c741c3f..683ccea668 100644 --- a/flytekit/core/container_task.py +++ b/flytekit/core/container_task.py @@ -129,8 +129,7 @@ def _render_command_and_volume_binding(self, cmd: str, **kwargs) -> Tuple[str, D """ We support template-style references to inputs, e.g., "{{.inputs.infile}}". - For FlyteFile and FlyteDirectory commands, e.g., "/var/inputs/inputs", we extract the key from strings that begin with the specified - `input_data_dir`. + For FlyteFile and FlyteDirectory commands, e.g., "/var/inputs/inputs", we extract the key from strings that begin with the specified `input_data_dir`. """ from flytekit.types.directory import FlyteDirectory from flytekit.types.file import FlyteFile From f9bd3b7250252e58a8ebf1992e78b09be53dc949 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Thu, 31 Oct 2024 19:36:48 -0700 Subject: [PATCH 05/12] ruff format Signed-off-by: wayner0628 --- flytekit/core/container_task.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/flytekit/core/container_task.py b/flytekit/core/container_task.py index 683ccea668..43f9ebe413 100644 --- a/flytekit/core/container_task.py +++ b/flytekit/core/container_task.py @@ -119,7 +119,7 @@ def _extract_path_command_key(self, cmd: str, input_data_dir: Optional[str]) -> import re input_data_dir = input_data_dir or "" - input_regex = fr"{re.escape(input_data_dir)}/(.+)$" + input_regex = rf"{re.escape(input_data_dir)}/(.+)$" match = re.match(input_regex, cmd) if match: return match.group(1) @@ -143,9 +143,11 @@ def _render_command_and_volume_binding(self, cmd: str, **kwargs) -> Tuple[str, D input_val = kwargs.get(k) if type(input_val) in [FlyteFile, FlyteDirectory]: if not path_k: - raise AssertionError("FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}") + raise AssertionError( + "FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" + ) local_flyte_file_or_dir_path = str(input_val) - remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k) # type: ignore + remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k) # type: ignore volume_binding[local_flyte_file_or_dir_path] = { "bind": remote_flyte_file_or_dir_path, "mode": "rw", From 456a1c93ea1c14155dfa581c94e8e0bbc9f111b7 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Sat, 2 Nov 2024 18:03:17 -0700 Subject: [PATCH 06/12] add wrong syntax unit testing Signed-off-by: wayner0628 --- .../unit/core/test_local_raw_container.py | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index ad42ad3c5c..b89d59854e 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -60,6 +60,35 @@ def flyte_file_io_wf() -> FlyteFile: assert content == "This is flyte_file.txt file." +@pytest.mark.skipif( + sys.platform in ["darwin", "win32"], + reason="Skip if running on windows or macos due to CI Docker environment setup failure", +) +def test_flytefile_wrong_syntax(): + client = docker.from_env() + path_to_dockerfile = "tests/flytekit/unit/core/" + dockerfile_name = "Dockerfile.raw_container" + client.images.build(path=path_to_dockerfile, dockerfile=dockerfile_name, tag="flytekit:rawcontainer") + + with pytest.raises( + AssertionError, match="FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" + ): + ContainerTask( + name="flyte_file_io", + input_data_dir="/var/inputs", + output_data_dir="/var/outputs", + inputs=kwtypes(inputs=FlyteFile), + outputs=kwtypes(out=FlyteFile), + image="flytekit:rawcontainer", + command=[ + "python", + "write_flytefile.py", + "{{.inputs.inputs}}", + "/var/outputs/out", + ], + ) + + @pytest.mark.skipif( sys.platform in ["darwin", "win32"], reason="Skip if running on windows or macos due to CI Docker environment setup failure", @@ -110,6 +139,35 @@ def flyte_dir_io_wf() -> FlyteDirectory: assert content == "This is for flyte dir." +@pytest.mark.skipif( + sys.platform in ["darwin", "win32"], + reason="Skip if running on windows or macos due to CI Docker environment setup failure", +) +def test_flytedir_wrong_syntax(): + client = docker.from_env() + path_to_dockerfile = "tests/flytekit/unit/core/" + dockerfile_name = "Dockerfile.raw_container" + client.images.build(path=path_to_dockerfile, dockerfile=dockerfile_name, tag="flytekit:rawcontainer") + + with pytest.raises( + AssertionError, match="FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" + ): + ContainerTask( + name="flyte_dir_io", + input_data_dir="/var/inputs", + output_data_dir="/var/outputs", + inputs=kwtypes(inputs=FlyteDirectory), + outputs=kwtypes(out=FlyteDirectory), + image="flytekit:rawcontainer", + command=[ + "python", + "write_flytedir.py", + "{{.inputs.inputs}}", + "/var/outputs/out", + ], + ) + + @pytest.mark.skipif( sys.platform in ["darwin", "win32"], reason="Skip if running on windows or macos due to CI Docker environment setup failure", From c783fda8f304ecd4534b52a6b924c9569fac3d30 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Sat, 2 Nov 2024 18:12:37 -0700 Subject: [PATCH 07/12] fix pytest raise Signed-off-by: wayner0628 --- .../unit/core/test_local_raw_container.py | 89 +++++++++++++------ 1 file changed, 61 insertions(+), 28 deletions(-) diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index b89d59854e..525d5801d3 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -70,23 +70,38 @@ def test_flytefile_wrong_syntax(): dockerfile_name = "Dockerfile.raw_container" client.images.build(path=path_to_dockerfile, dockerfile=dockerfile_name, tag="flytekit:rawcontainer") + flyte_file_io = ContainerTask( + name="flyte_file_io", + input_data_dir="/var/inputs", + output_data_dir="/var/outputs", + inputs=kwtypes(inputs=FlyteFile), + outputs=kwtypes(out=FlyteFile), + image="flytekit:rawcontainer", + command=[ + "python", + "write_flytefile.py", + "{{.inputs.inputs}}", + "/var/outputs/out", + ], + ) + + @task + def flyte_file_task() -> FlyteFile: + working_dir = flytekit.current_context().working_directory + write_file = os.path.join(working_dir, "flyte_file.txt") + with open(write_file, "w") as file: + file.write("This is flyte_file.txt file.") + return FlyteFile(path=write_file) + + @workflow + def flyte_file_io_wf() -> FlyteFile: + ff = flyte_file_task() + return flyte_file_io(inputs=ff) + with pytest.raises( AssertionError, match="FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" ): - ContainerTask( - name="flyte_file_io", - input_data_dir="/var/inputs", - output_data_dir="/var/outputs", - inputs=kwtypes(inputs=FlyteFile), - outputs=kwtypes(out=FlyteFile), - image="flytekit:rawcontainer", - command=[ - "python", - "write_flytefile.py", - "{{.inputs.inputs}}", - "/var/outputs/out", - ], - ) + flyte_file_io_wf() @pytest.mark.skipif( @@ -149,23 +164,41 @@ def test_flytedir_wrong_syntax(): dockerfile_name = "Dockerfile.raw_container" client.images.build(path=path_to_dockerfile, dockerfile=dockerfile_name, tag="flytekit:rawcontainer") + flyte_dir_io = ContainerTask( + name="flyte_dir_io", + input_data_dir="/var/inputs", + output_data_dir="/var/outputs", + inputs=kwtypes(inputs=FlyteDirectory), + outputs=kwtypes(out=FlyteDirectory), + image="flytekit:rawcontainer", + command=[ + "python", + "write_flytedir.py", + "{{.inputs.inputs}}", + "/var/outputs/out", + ], + ) + + @task + def flyte_dir_task() -> FlyteDirectory: + working_dir = flytekit.current_context().working_directory + local_dir = Path(os.path.join(working_dir, "csv_files")) + local_dir.mkdir(exist_ok=True) + write_file = os.path.join(local_dir, "flyte_dir.txt") + with open(write_file, "w") as file: + file.write("This is for flyte dir.") + + return FlyteDirectory(path=str(local_dir)) + + @workflow + def flyte_dir_io_wf() -> FlyteDirectory: + fd = flyte_dir_task() + return flyte_dir_io(inputs=fd) + with pytest.raises( AssertionError, match="FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" ): - ContainerTask( - name="flyte_dir_io", - input_data_dir="/var/inputs", - output_data_dir="/var/outputs", - inputs=kwtypes(inputs=FlyteDirectory), - outputs=kwtypes(out=FlyteDirectory), - image="flytekit:rawcontainer", - command=[ - "python", - "write_flytedir.py", - "{{.inputs.inputs}}", - "/var/outputs/out", - ], - ) + flyte_dir_io_wf() @pytest.mark.skipif( From 90510c2991b85fa6f1ee06f76de7482bf8ecf1e6 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Thu, 14 Nov 2024 13:30:48 +0800 Subject: [PATCH 08/12] Better Error MSG, very nice PR Wayner Signed-off-by: Future-Outlier --- flytekit/core/container_task.py | 4 +++- tests/flytekit/unit/core/test_local_raw_container.py | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/flytekit/core/container_task.py b/flytekit/core/container_task.py index 43f9ebe413..e2a3598aa4 100644 --- a/flytekit/core/container_task.py +++ b/flytekit/core/container_task.py @@ -144,7 +144,9 @@ def _render_command_and_volume_binding(self, cmd: str, **kwargs) -> Tuple[str, D if type(input_val) in [FlyteFile, FlyteDirectory]: if not path_k: raise AssertionError( - "FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" + "FlyteFile and FlyteDirectory commands should not use the template syntax like this: {{.inputs.infile}}\n" + "Please use a path-like syntax, such as: /var/inputs/infile.\n" + "This requirement is due to how Flyte Propeller processes template syntax inputs." ) local_flyte_file_or_dir_path = str(input_val) remote_flyte_file_or_dir_path = os.path.join(self._input_data_dir, k) # type: ignore diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index 525d5801d3..9fb49b5b90 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -99,7 +99,10 @@ def flyte_file_io_wf() -> FlyteFile: return flyte_file_io(inputs=ff) with pytest.raises( - AssertionError, match="FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" + AssertionError, + match="FlyteFile and FlyteDirectory commands should not use the template syntax like this: {{.inputs.infile}}\n" + "Please use a path-like syntax, such as: /var/inputs/infile.\n" + "This requirement is due to how Flyte Propeller processes template syntax inputs." ): flyte_file_io_wf() From 8917bb63a74d330c0eef3fc3eca81245c833cafb Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Thu, 14 Nov 2024 13:53:03 +0800 Subject: [PATCH 09/12] nit Signed-off-by: Future-Outlier --- tests/flytekit/unit/core/test_local_raw_container.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index 9fb49b5b90..e92ef34efc 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -100,9 +100,9 @@ def flyte_file_io_wf() -> FlyteFile: with pytest.raises( AssertionError, - match="FlyteFile and FlyteDirectory commands should not use the template syntax like this: {{.inputs.infile}}\n" - "Please use a path-like syntax, such as: /var/inputs/infile.\n" - "This requirement is due to how Flyte Propeller processes template syntax inputs." + match=r"FlyteFile and FlyteDirectory commands should not use the template syntax like this: {{.inputs.infile}}\n" + r"Please use a path-like syntax, such as: /var/inputs/infile.\n" + r"This requirement is due to how Flyte Propeller processes template syntax inputs." ): flyte_file_io_wf() From dd40447d32ab3cc9e1191a3bc20b483f91f88492 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Thu, 14 Nov 2024 13:54:02 +0800 Subject: [PATCH 10/12] nit Signed-off-by: Future-Outlier --- tests/flytekit/unit/core/test_local_raw_container.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index e92ef34efc..75bc502117 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -100,13 +100,14 @@ def flyte_file_io_wf() -> FlyteFile: with pytest.raises( AssertionError, - match=r"FlyteFile and FlyteDirectory commands should not use the template syntax like this: {{.inputs.infile}}\n" + match=( + r"FlyteFile and FlyteDirectory commands should not use the template syntax like this: \{\{\.inputs\.infile\}\}\n" r"Please use a path-like syntax, such as: /var/inputs/infile.\n" r"This requirement is due to how Flyte Propeller processes template syntax inputs." + ) ): flyte_file_io_wf() - @pytest.mark.skipif( sys.platform in ["darwin", "win32"], reason="Skip if running on windows or macos due to CI Docker environment setup failure", From 9416ca3904720393d0924d5aef442c667ec1ee33 Mon Sep 17 00:00:00 2001 From: wayner0628 Date: Wed, 13 Nov 2024 22:00:14 -0800 Subject: [PATCH 11/12] nit Signed-off-by: wayner0628 --- tests/flytekit/unit/core/test_local_raw_container.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/flytekit/unit/core/test_local_raw_container.py b/tests/flytekit/unit/core/test_local_raw_container.py index 75bc502117..5b4418d703 100644 --- a/tests/flytekit/unit/core/test_local_raw_container.py +++ b/tests/flytekit/unit/core/test_local_raw_container.py @@ -200,7 +200,12 @@ def flyte_dir_io_wf() -> FlyteDirectory: return flyte_dir_io(inputs=fd) with pytest.raises( - AssertionError, match="FlyteFile and FlyteDirectory commands should not use the syntax: {{.inputs.infile}}" + AssertionError, + match=( + r"FlyteFile and FlyteDirectory commands should not use the template syntax like this: \{\{\.inputs\.infile\}\}\n" + r"Please use a path-like syntax, such as: /var/inputs/infile.\n" + r"This requirement is due to how Flyte Propeller processes template syntax inputs." + ) ): flyte_dir_io_wf() From 1970cb8736d7636ebe005ffa44a31c2cee98aa5d Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Thu, 14 Nov 2024 21:06:33 +0800 Subject: [PATCH 12/12] Trigger CI Signed-off-by: Future-Outlier