Added glob support for Snowpark and Streamlit #1814

sfc-gh-astus · 2024-10-29T12:47:53Z

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

Introduced Artifacts type and changed models for Snowpark, Streamlit and Native App

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

sfc-gh-mraba · 2024-10-29T15:36:21Z

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

+        for artifact in artifacts:
+            if isinstance(artifact, PathMapping):
+                _artifacts.append(artifact)
            else:
-                _artifacts.append(PathMapping(src=artefact))
+                _artifacts.append(PathMapping(src=artifact))


I believe that previous name was correct.

'Artefact' is the British spelling, while, 'artifact' is the American spelling.

We have both versions in code.

sfc-gh-mraba · 2024-10-29T15:42:43Z

tests/test_data/projects/glob_patterns/pages/my_page.py

Why my_page.py is empty file?

mypy had problems with non existing title function.

tests_integration/test_data/projects/snowpark_glob_patterns/requirements.txt

tests/snowpark/test_project_paths.py

sfc-gh-astus · 2024-10-31T16:14:32Z

Snowpark output directory per stage or leave it for diff/sync PR?

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

sfc-gh-jsikorski · 2024-11-05T11:47:14Z

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

 from typing import Dict, Optional, Set, Tuple

 import typer
 from click import ClickException, UsageError
+from snowflake.cli._plugins.nativeapp.artifacts import BundleMap, symlink_or_copy


If we're going to use BundleMap for every type of entity, maybe it's best to move it from native app plugin?

It is good idea, but not in this PR.

sfc-gh-jsikorski · 2024-11-05T12:16:12Z

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

 )
+from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import PathMapping


Again, maybe we could extract common objects, instead of cross-plugin imports

Definitely, but not in this PR.

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

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

src/snowflake/cli/api/feature_flags.py

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

tests/helpers/__snapshots__/test_v1_to_v2.ambr

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

sfc-gh-bdufour · 2024-11-06T15:30:14Z

src/snowflake/cli/_plugins/nativeapp/artifacts.py

@@ -303,19 +303,18 @@ def _add(self, src: Path, dest: Path, map_as_child: bool) -> None:
            src=canonical_src, dest=canonical_dest, dest_is_dir=dest_is_dir
        )

-    def _add_mapping(self, src: str, dest: Optional[str] = None):
+    def _add_mapping(self, src: Path, dest: Optional[str] = None):


src was a string here because it can be a glob pattern as well, which is odd to represent as a Path. I'm not a big fan of this change (or the matching one in the model)

After discussion we decided to back to string.

sfc-gh-bdufour · 2024-11-06T16:28:42Z

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

@@ -225,8 +228,8 @@ def build_artifacts_mappings(
            entities_to_imports_map[entity_id].add(artefact_dto.import_path(stage))
        stages_to_artifact_map[stage].update(required_artifacts)

-        if project_paths.dependencies.exists():
-            deps_artefact = project_paths.get_dependencies_artefact()
+        deps_artefact = project_paths.get_dependencies_artefact()


nit: we should pick one of british or american spelling for artifact/artefact and use it consistently

I agree, created ticket for it.

sfc-gh-bdufour · 2024-11-06T16:30:37Z

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

+                    deploy_root=(project_paths.project_root / "output").absolute(),
+                )
+                bundle_map.add(
+                    PathMapping(src=str(artefact.path), dest=(artefact.dest or None))


Ah, commented above before seeing this conversation. I think representing a glob pattern as a Path is misleading, I'm not in favor. There's a reason we were using str explicitly, as opposed to Path pretty much everywhere else when a Path is resolved. If we want to make this "might be a glob" explicit, happy to consider something like a type alias or similar.

sfc-gh-turbaszek · 2024-11-07T12:06:33Z

| If we want to make this "might be a glob" explicit, happy to consider something like a type alias or similar.

+1 to this idea I think it would make sense to have PathOrGlob = str

sfc-gh-bdufour · 2024-11-07T13:22:39Z

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

-                _artifacts.append(artefact)
+        for artifact in artifacts:
+            if (
+                "*" in artifact


* isn't the only glob pattern, so that's a really weak check. What about something like file[1-5].py ?

I wanted to to start with basics, I will try with glob.has_magic.

I tried and it works after small changes!

sfc-gh-bdufour · 2024-11-07T13:24:59Z

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

+        for artifact in artifacts:
+            if (
+                "*" in artifact
+                and FeatureFlag.ENABLE_SNOWPARK_BUNDLE_MAP_BUILD.is_disabled()


nit: not generally a fan of naming publicly visible concepts by implementation details like "bundle map". Is there a more user-friendly name we could use here?

Also, is it not the default because of backwards compat concerns? I wonder if the solution isn't to make it default, but then give a flag to revert to old behaviour if needed (SNOWPARK_LEGACY_GLOB_BEHAVIOUR=true ?)

It can not be default, because it is BCR. New build changes zip paths.

Maybe ENABLE_SNOWPARK_GLOB_SUPPORT?

sfc-gh-bdufour · 2024-11-07T13:26:41Z

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

+                    for (absolute_src, absolute_dest) in bundle_map.all_mappings(
+                        absolute=True, expand_directories=False
+                    ):
+                        symlink_or_copy(


we'll need to apply processors as well, is that planned for a future PR? I can probably help migrate processors to a more general location (and the bundle map too while we're at it), if you want the help. I think there are a few things to fix with processors in the process anyway, this is a good opportunity to make those small, under-the-hood changes.

I totally forgot about processors, but I thing it should be added in new PR.

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

sfc-gh-bdufour

None of my comments are blockers, so LGTM for NADE

This reverts commit 3eb543d.

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

sfc-gh-bdufour · 2024-11-15T12:51:40Z

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

+    def deploy_root(self) -> Path:
+        return self.project_root / "output"


this could cause conflicts with other domains, since native apps uses output/deploy by default. We make ours configurable as well through snowflake.yml. We should probably align the choice of deploy root, and make it per-entity so that multiple entities, even across different domains, don't negatively interfere with each other. We should probably do that as a follow-up PR very quickly.

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

sfc-gh-bdufour · 2024-11-15T12:53:50Z

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

+
+    @property
+    def deploy_root(self) -> Path:
+        return self.project_root / "output"


native apps uses output/deploy for this. We have other ephemeral directories under output/ as well, e.g. output/bundle as the bundle root. We also make all of those configurable.

sfc-gh-mraba reviewed Oct 29, 2024

View reviewed changes

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

sfc-gh-mraba reviewed Oct 29, 2024

View reviewed changes

tests_integration/test_data/projects/snowpark_glob_patterns/requirements.txt Show resolved Hide resolved

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch 16 times, most recently from 3653ce8 to 7ef640a Compare October 31, 2024 12:08

sfc-gh-astus commented Oct 31, 2024

View reviewed changes

tests/snowpark/test_project_paths.py Outdated Show resolved Hide resolved

sfc-gh-astus commented Nov 5, 2024

View reviewed changes

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

sfc-gh-astus commented Nov 5, 2024

View reviewed changes

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

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch from 7ef640a to 4c3ee60 Compare November 5, 2024 09:37

sfc-gh-astus marked this pull request as ready for review November 5, 2024 09:52

sfc-gh-astus requested review from a team as code owners November 5, 2024 09:52

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch 2 times, most recently from 5a0fc07 to c8058c6 Compare November 5, 2024 10:56

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch from eebdb30 to 19fdf46 Compare November 5, 2024 12:45

sfc-gh-jsikorski reviewed Nov 5, 2024

View reviewed changes

sfc-gh-pjob reviewed Nov 6, 2024

View reviewed changes

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch from 2e7278d to 6968934 Compare November 6, 2024 16:32

sfc-gh-bdufour reviewed Nov 6, 2024

View reviewed changes

sfc-gh-turbaszek closed this Nov 7, 2024

sfc-gh-turbaszek reopened this Nov 7, 2024

sfc-gh-bdufour reviewed Nov 7, 2024

View reviewed changes

sfc-gh-bdufour previously approved these changes Nov 7, 2024

View reviewed changes

sfc-gh-astus dismissed sfc-gh-bdufour’s stale review via 1e1f125 November 7, 2024 15:06

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch 2 times, most recently from 4e569bd to 7735817 Compare November 14, 2024 11:10

sfc-gh-astus added 11 commits November 14, 2024 17:35

Added glob support for Snowpark and Streamlit

e701302

Added glob support for Snowpark and Streamlit

576dcee

Added glob support for Snowpark and Streamlit

2c7cafa

Changed PathMapping.src to Path

8725d8b

Changes after review

6ea5273

Changes after review 2

26c1e9d

Revert "Changed PathMapping.src to Path"

5a4528f

This reverts commit 3eb543d.

Changes after review 3

943bc62

Changes after review 4

e923420

Added full global support

ea2fb69

Renamed feature flag and added release notes

91abd4d

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch from 7735817 to 4a558d8 Compare November 14, 2024 16:36

sfc-gh-mraba previously approved these changes Nov 14, 2024

View reviewed changes

sfc-gh-bdufour reviewed Nov 15, 2024

View reviewed changes

Removed or None

cfb17bf

sfc-gh-astus dismissed sfc-gh-mraba’s stale review via cfb17bf November 15, 2024 13:37

sfc-gh-astus force-pushed the added-glob-support-for-snowpark-and-streamlit branch from 4a558d8 to cfb17bf Compare November 15, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added glob support for Snowpark and Streamlit #1814

Added glob support for Snowpark and Streamlit #1814

sfc-gh-astus commented Oct 29, 2024 •

edited

Loading

sfc-gh-mraba Oct 29, 2024

sfc-gh-astus Oct 30, 2024

sfc-gh-mraba Oct 29, 2024

sfc-gh-astus Oct 30, 2024

sfc-gh-astus commented Oct 31, 2024

sfc-gh-jsikorski Nov 5, 2024

sfc-gh-astus Nov 5, 2024 •

edited

Loading

sfc-gh-jsikorski Nov 5, 2024

sfc-gh-astus Nov 5, 2024

sfc-gh-bdufour Nov 6, 2024

sfc-gh-astus Nov 7, 2024

sfc-gh-bdufour Nov 6, 2024

sfc-gh-astus Nov 7, 2024

sfc-gh-bdufour Nov 6, 2024

sfc-gh-turbaszek commented Nov 7, 2024

sfc-gh-bdufour Nov 7, 2024

sfc-gh-astus Nov 7, 2024 •

edited

Loading

sfc-gh-astus Nov 12, 2024

sfc-gh-bdufour Nov 7, 2024

sfc-gh-astus Nov 7, 2024

sfc-gh-astus Nov 14, 2024 •

edited

Loading

sfc-gh-bdufour Nov 7, 2024

sfc-gh-astus Nov 7, 2024

sfc-gh-bdufour left a comment

sfc-gh-bdufour Nov 15, 2024

sfc-gh-bdufour Nov 15, 2024

		)
		from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import PathMapping

		def deploy_root(self) -> Path:
		return self.project_root / "output"

Added glob support for Snowpark and Streamlit #1814

Are you sure you want to change the base?

Added glob support for Snowpark and Streamlit #1814

Conversation

sfc-gh-astus commented Oct 29, 2024 • edited Loading

Pre-review checklist

Changes description

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 Oct 31, 2024

Choose a reason for hiding this comment

sfc-gh-astus Nov 5, 2024 • edited Loading

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-turbaszek commented Nov 7, 2024

Choose a reason for hiding this comment

sfc-gh-astus Nov 7, 2024 • edited Loading

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 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-bdufour left a 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 Oct 29, 2024 •

edited

Loading

sfc-gh-astus Nov 5, 2024 •

edited

Loading

sfc-gh-astus Nov 7, 2024 •

edited

Loading

sfc-gh-astus Nov 14, 2024 •

edited

Loading