Features/unification artifacts #1920

sfc-gh-astus · 2024-12-04T17:04:15Z

Pre-review checklist

I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
I've added or updated automated unit tests to verify correctness of my new code.
I've added or updated integration tests to verify correctness of my new code.
I've confirmed that my changes are working by executing CLI's commands manually on MacOS.
I've confirmed that my changes are working by executing CLI's commands manually on Windows.
I've confirmed that my changes are up-to-date with the target branch.
I've described my changes in the release notes.
I've described my changes in the section below.

Changes description

Added support for glob pattern in artifact paths in snowflake.yml for Streamlit.
Added support for glob pattern in artifact paths in snowflake.yml for Snowpark, requires ENABLE_SNOWPARK_GLOB_SUPPORT feature flag.
Extracted artifacts common logic to common package.

sfc-gh-astus · 2024-12-12T12:26:17Z

Check if --project in streamlit works on this branch.

sfc-gh-pczajka · 2025-01-09T16:51:50Z

src/snowflake/cli/_plugins/snowpark/snowpark_entity_model.py

+                or (isinstance(artifact, PathMapping) and glob.has_magic(artifact.src))
+            ) and FeatureFlag.ENABLE_SNOWPARK_GLOB_SUPPORT.is_disabled():
+                raise ValueError(
+                    "If you want to use glob patterns in artifacts, you need to enable the Snowpark new build feature flag (ENABLE_SNOWPARK_GLOB_SUPPORT=true)"


Suggested change

"If you want to use glob patterns in artifacts, you need to enable the Snowpark new build feature flag (ENABLE_SNOWPARK_GLOB_SUPPORT=true)"

"If you want to use glob patterns in artifacts, you need to enable the Snowpark new build feature flag (enable_snowpark_glob_support=true)"

I'm pretty sure that feature flags are read only as lowercase from config.toml. SNOWFLAKE_CLI_FEATURES_ENABLE... env variable also works

sfc-gh-pczajka · 2025-01-10T13:46:31Z

src/snowflake/cli/api/project/project_paths.py

+
+    def remove_up_bundle_root(self) -> None:
+        if self.bundle_root.exists():
+            rmtree(self.bundle_root)


Use SecurePath.rmdir(recursive=True) instead of shutil.rmtree

sfc-gh-pczajka · 2025-01-10T13:48:05Z

src/snowflake/cli/api/artifacts/utils.py

+            ssrc.copy(dst)
+    else:
+        # 1. Create a new directory in the deploy root
+        dst.mkdir(exist_ok=True)


Suggested change

dst.mkdir(exist_ok=True)

SecurePath(dst).mkdir(exist_ok=True)

sfc-gh-pczajka · 2025-01-10T13:49:21Z

src/snowflake/cli/api/artifacts/utils.py

+        for root, _, files in sorted(os.walk(absolute_src, followlinks=True)):
+            relative_root = Path(root).relative_to(absolute_src)
+            absolute_root_in_deploy = Path(dst, relative_root)
+            absolute_root_in_deploy.mkdir(parents=True, exist_ok=True)


Suggested change

absolute_root_in_deploy.mkdir(parents=True, exist_ok=True)

SecurePath(absolute_root_in_deploy).mkdir(parents=True, exist_ok=True)

sfc-gh-jsikorski · 2025-01-09T10:57:09Z

src/snowflake/cli/api/artifacts/bundle_map.py

+class BundleMap:
+    """
+    Computes the mapping between project directory artifacts (aka source artifacts) to their deploy root location
+    (aka destination artifact). This information is primarily used when bundling a native applications project.


Docstring mentions Native Apps- we probably should make it CLI-wide

sfc-gh-jsikorski · 2025-01-10T08:19:03Z

src/snowflake/cli/api/artifacts/bundle_map.py

+
+    def __init__(self, *, project_root: Path, deploy_root: Path):
+        # If a relative path ends up here, it's a bug in the app and can lead to other
+        # subtle bugs as paths would be resolved relative to the current working directory.


Is this docstring just copied from Native App application?

sfc-gh-jsikorski · 2025-01-10T08:34:47Z

src/snowflake/cli/api/artifacts/bundle_map.py

+        Returns the canonical version of a source path, relative to the project root.
+        """
+        absolute_src = self._absolute_src(src)
+        return absolute_src.relative_to(self._project_root)


What if artifact is in a different subtree? Do we handle the error?

This is nativeapp code moved to common package.

sfc-gh-jsikorski · 2025-01-10T08:49:50Z

src/snowflake/cli/api/artifacts/bundle_map.py

+    :param deploy_root: The directory where artifacts should be copied to. Must be an absolute path.
+    """
+
+    def __init__(self, *, project_root: Path, deploy_root: Path):


@sfc-gh-pczajka - shouldn't we use SecurePath everywhere? What's your opinion on this?

Wherever possible - so if it won't cause cascade of changes, I'd change Path to SecurePath.
I remember that refactor to use it everywhere was problematic

It will cause cascade :( , there will be a lot of changes in nativeapp code.

sfc-gh-jsikorski · 2025-01-10T13:16:06Z

src/snowflake/cli/_plugins/snowpark/snowpark_project_paths.py



 @dataclass(unsafe_hash=True)
 class Artefact:
    """Helper for getting paths related to given artefact."""

+    project_root: Path


Can we move this logic to main bundle map files? It seems quite similiar

Artefact logic is only for Snowpark, for me better fits here.

sfc-gh-jsikorski · 2025-01-10T14:44:17Z

src/snowflake/cli/_plugins/streamlit/streamlit_entity.py

+                )
+                for artifact in self._entity_model.artifacts
+            ],
+        )


Bundle is an action, so it should not return bundle but rather copy the files

* Added glob support for Snowpark and Streamlit * Added glob support for Snowpark and Streamlit * Added glob support for Snowpark and Streamlit * Changed PathMapping.src to Path * Changes after review * Changes after review 2 * Revert "Changed PathMapping.src to Path" This reverts commit 3eb543d. * Changes after review 3 * Changes after review 4 * Added full global support * Renamed feature flag and added release notes * Removed or None

Fix feature branch gates

#1909) Moved Snowpark and Streamlit artifacts to separate directory in output/deploy

sfc-gh-astus requested review from a team as code owners December 4, 2024 17:04

sfc-gh-bdufour force-pushed the features/unification-artifacts branch 2 times, most recently from 85d1c72 to 9cb87b3 Compare December 9, 2024 21:15

sfc-gh-astus force-pushed the features/unification-artifacts branch from bf62cfb to 7cd92a3 Compare December 12, 2024 17:53

sfc-gh-astus changed the title ~~[DO NOT MERGE] Features/unification artifacts~~ Features/unification artifacts Dec 12, 2024

sfc-gh-astus force-pushed the features/unification-artifacts branch from 2bee685 to c15cca5 Compare December 13, 2024 10:07

sfc-gh-astus force-pushed the features/unification-artifacts branch 2 times, most recently from 1ce276f to 1d6e6c1 Compare January 8, 2025 13:54

sfc-gh-pczajka reviewed Jan 9, 2025

View reviewed changes

sfc-gh-pczajka reviewed Jan 10, 2025

View reviewed changes

sfc-gh-jsikorski requested changes Jan 10, 2025

View reviewed changes

sfc-gh-astus and others added 11 commits January 13, 2025 14:54

Fix broken v1 to v2 test on the feature branch (#1876)

2610a66

Moved native app artifacts logic to common API (#1877)

5c97aee

Move PathMapping and ProcessorMapping out of native app logic (#1899)

552ff2a

Fix rebase issues in feature/unification-artifacts (#1932)

2eb7fe7

Fix feature branch gates

Fixes to imports following rebase

e4b8077

Moved Snowpark and Streamlit artifacts to separate directory in outpu… (

dc73f89

#1909) Moved Snowpark and Streamlit artifacts to separate directory in output/deploy

Updated release-notes

9ea7b5a

Fixes after rebase

a0a29ff

Fixes after rebase

83302c3

Fixes after review

517a85f

sfc-gh-astus force-pushed the features/unification-artifacts branch from 1d6e6c1 to 517a85f Compare January 13, 2025 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/unification artifacts #1920

Features/unification artifacts #1920

sfc-gh-astus commented Dec 4, 2024 •

edited

Loading

sfc-gh-astus commented Dec 12, 2024

sfc-gh-pczajka Jan 9, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-pczajka Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-pczajka Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-pczajka Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-jsikorski Jan 9, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-jsikorski Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-jsikorski Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-jsikorski Jan 10, 2025

sfc-gh-pczajka Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-jsikorski Jan 10, 2025

sfc-gh-astus Jan 13, 2025

sfc-gh-jsikorski Jan 10, 2025

sfc-gh-astus Jan 13, 2025

	"If you want to use glob patterns in artifacts, you need to enable the Snowpark new build feature flag (ENABLE_SNOWPARK_GLOB_SUPPORT=true)"
	"If you want to use glob patterns in artifacts, you need to enable the Snowpark new build feature flag (enable_snowpark_glob_support=true)"

	dst.mkdir(exist_ok=True)
	SecurePath(dst).mkdir(exist_ok=True)

	absolute_root_in_deploy.mkdir(parents=True, exist_ok=True)
	SecurePath(absolute_root_in_deploy).mkdir(parents=True, exist_ok=True)

Features/unification artifacts #1920

Are you sure you want to change the base?

Features/unification artifacts #1920

Conversation

sfc-gh-astus commented Dec 4, 2024 • edited Loading

Pre-review checklist

Changes description

sfc-gh-astus commented Dec 12, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-astus commented Dec 4, 2024 •

edited

Loading