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.

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

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

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

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

sfc-gh-turbaszek · 2025-01-15T08:14:48Z

RELEASE-NOTES.md

@@ -41,6 +41,8 @@
 * Add `snow spcs service metrics` command to fetch service metrics:
  * Supports filtering by service name, container name, instance ID, and time intervals (`--since`, `--until`).
  * Use `--all` to fetch all columns.
+* 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.


Is there any change to existing behavior?

Streamlit is changed, Snowpark needs feature flag enabled, otherwise stays the same.

sfc-gh-pczajka · 2025-01-15T09:38:11Z

tests/snowpark/test_artifacts.py

+@pytest.mark.parametrize(
+    "artifact, local_path, stage_path",
+    [
+        ("src", bundle_root / "src.zip", "/"),


All paths in tests seem to start on src/. Do we support glob patterns like **/snowpark/*.py?

No, ** introduce more complexity, so I decided to not add that. This PR is huge even without **.

I added that to release notes.

RELEASE-NOTES.md

src/snowflake/cli/api/project/schemas/entities/common.py

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

sfc-gh-melnacouzi · 2025-01-15T19:21:36Z

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

@@ -214,6 +222,43 @@ def _standardize(packages: List[str]) -> Set[str]:
    return _standardize(old_dependencies) != _standardize(new_dependencies)


+def map_path_mapping_to_artifact(
+    project_paths: SnowparkProjectPaths, artifacts: List[PathMapping]
+) -> List[Artefact]:


minor: would be nice to use one spelling for Artifact throughout the code base :)

I will do this in next PR.

sfc-gh-melnacouzi · 2025-01-15T19:25:28Z

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

+            if (
+                (isinstance(artifact, str) and glob.has_magic(artifact))
+                or (isinstance(artifact, PathMapping) and glob.has_magic(artifact.src))
+            ) and FeatureFlag.ENABLE_SNOWPARK_GLOB_SUPPORT.is_disabled():


How do we know if user is intending to use glob pattern or not? If we can know the intent, then what's the purpose of the feature flag?

I check if artifact path has glob magic. Feature flag changes build and deploy commands behaviour, they use deploy/bundle/... directory.

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

sfc-gh-melnacouzi · 2025-01-15T19:27:44Z

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

+            return False
+        return re.search(r"\.[a-zA-Z0-9]{2,4}$", self.dest) is not None
+
+    def _path_until_asterisk(self) -> Path:


For this glob logic, isn't it already implement for native apps?

I don't understand the question.

Sorry let me rephrase. This Artifact class, can't it also be used for Native Apps and other entities? If not, what's special about Snowpark artifacts?

In Snowpark we build zip files for artifacts which are not single file. It is easier to import one zip file than multiple py files. Artifact class should be used only in Snowpark.

sfc-gh-melnacouzi · 2025-01-15T19:29:36Z

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

-                )
-
-        return self
+    @field_validator("artifacts")


why aren't we using ArtifactsBase?

What is ArtefactsBase? Can't find in code.

sorry I meant ArtifactsBaseModel (now EntityModelBaseWithArtifacts)

Good point! There is no problem with Snowpark, but Streamlit's artifacts were optional and when I changed to EntityModelBaseWithArtifacts now are required. It is BCR, I will talk with the team what we want to do with it.

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

sfc-gh-melnacouzi · 2025-01-15T20:07:48Z

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

+            SecurePath(self.bundle_root).rmdir(recursive=True)
+
+
+def bundle_root(root: Path, app_type: str | None = None) -> Path:


why is app_type in the path?

To separate Streamlit and Snowpark artifacts, you can run

snowpark build streamlit deploy snowpark deploy

shouldn't we use entity ids instead, since you have multiple entities of each?

There was that idea before but would introduce more complexity, so we decided to go simple way at the beginning.

* 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

src/snowflake/cli/_plugins/snowpark/snowpark_entity_model.py Outdated Show resolved Hide resolved

sfc-gh-pczajka reviewed Jan 10, 2025

View reviewed changes

src/snowflake/cli/api/project/project_paths.py Outdated Show resolved Hide resolved

sfc-gh-pczajka reviewed Jan 10, 2025

View reviewed changes

src/snowflake/cli/api/artifacts/utils.py Outdated Show resolved Hide resolved

sfc-gh-pczajka reviewed Jan 10, 2025

View reviewed changes

src/snowflake/cli/api/artifacts/utils.py Outdated Show resolved Hide resolved

sfc-gh-jsikorski requested changes Jan 10, 2025

View reviewed changes

sfc-gh-astus force-pushed the features/unification-artifacts branch 3 times, most recently from 878d715 to 9b6b495 Compare January 14, 2025 14:18

sfc-gh-turbaszek reviewed Jan 15, 2025

View reviewed changes

sfc-gh-pczajka reviewed Jan 15, 2025

View reviewed changes

sfc-gh-jsikorski self-requested a review January 15, 2025 10:40

sfc-gh-jsikorski previously approved these changes Jan 15, 2025

View reviewed changes

sfc-gh-astus dismissed sfc-gh-jsikorski’s stale review via 1169da8 January 15, 2025 10:55

sfc-gh-astus force-pushed the features/unification-artifacts branch from 1169da8 to 1a17d7e Compare January 15, 2025 10:55

sfc-gh-pczajka previously approved these changes Jan 15, 2025

View reviewed changes

sfc-gh-jsikorski self-requested a review January 15, 2025 13:05

sfc-gh-jsikorski previously approved these changes Jan 15, 2025

View reviewed changes

sfc-gh-melnacouzi reviewed Jan 15, 2025

View reviewed changes

sfc-gh-astus and others added 2 commits January 16, 2025 13:15

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

93aabdc

sfc-gh-bdufour and others added 12 commits January 16, 2025 13:15

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

f9aaa4b

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

813e766

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

58e5e4d

Fix feature branch gates

Fixes to imports following rebase

2b374b1

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

a6ef4a8

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

Updated release-notes

0d63f77

Fixes after rebase

14222a0

Fixes after rebase

28e06a8

Fixes after review

066ebf0

Fixes after rebase

8a916dc

Updated release notes

d1fe0ce

Fixes after review

6ef2d27

sfc-gh-astus dismissed stale reviews from sfc-gh-jsikorski and sfc-gh-pczajka via 6ef2d27 January 16, 2025 12:15

sfc-gh-astus force-pushed the features/unification-artifacts branch from 1a17d7e to 6ef2d27 Compare January 16, 2025 12:15

sfc-gh-melnacouzi previously approved these changes Jan 16, 2025

View reviewed changes

Fixes after review

02cb662

sfc-gh-astus dismissed sfc-gh-melnacouzi’s stale review via 02cb662 January 16, 2025 15:51

sfc-gh-pczajka approved these changes Jan 16, 2025

View reviewed changes

sfc-gh-melnacouzi approved these changes Jan 16, 2025

View reviewed changes

sfc-gh-jsikorski approved these changes Jan 17, 2025

View reviewed changes

sfc-gh-astus merged commit 1556a0d into main Jan 17, 2025
34 checks passed

sfc-gh-astus deleted the features/unification-artifacts branch January 17, 2025 09:06

sfc-gh-jsikorski mentioned this pull request Jan 17, 2025

Depends on in snowflake.yml #1993

Open

8 tasks

sfc-gh-astus mentioned this pull request Jan 24, 2025

SNOW-1446046: Support glob() for additional_source_files in streamlit deployment. #1108

Closed

		SecurePath(self.bundle_root).rmdir(recursive=True)


		def bundle_root(root: Path, app_type: str \| None = None) -> Path:

Features/unification artifacts #1920

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

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