V2.0 Definition for snowpark #1402

sfc-gh-jsikorski · 2024-08-01T07:25:06Z

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

Creating V2.0 project definition for Snowpark entities

# Conflicts: # src/snowflake/cli/plugins/snowpark/commands.py # src/snowflake/cli/plugins/streamlit/commands.py # src/snowflake/cli/templates/default_snowpark/snowflake.yml # src/snowflake/cli/templates/default_streamlit/snowflake.yml

# Conflicts: # src/snowflake/cli/api/feature_flags.py # src/snowflake/cli/api/project/schemas/entities/entities.py # src/snowflake/cli/api/project/schemas/entities/streamlit_entity.py # src/snowflake/cli/api/project/schemas/project_definition.py # tests/streamlit/__snapshots__/test_commands.ambr # tests/streamlit/test_commands.py

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

Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>

sfc-gh-turbaszek · 2024-08-01T09:18:02Z

src/snowflake/cli/api/project/schemas/project_definition.py

+EntityOrList = Union[
+    AnnotatedEntity, List[AnnotatedEntity]
+]  # TODO: Probablyu should delete this line
+


Is this change neccessary? Are you building on current main?

It's just for simplicity - same thing is used 4 times, so seems reasonable to put it into variable

sfc-gh-turbaszek · 2024-08-02T07:40:55Z

src/snowflake/cli/api/project/schemas/project_definition.py

+    defaults: Optional[DefaultsField] = Field(
+        title="Default key/value entity values that are merged recursively for each entity.",
+        default=None,
+    )
+
+    env: Optional[Dict[str, Union[str, int, bool]]] = Field(
+        title="Default environment specification for this project.",
+        default=None,


Is this really being added in this PR? How does it releat to Snowpark?

It was moved from line 165. It seems a bit more consistent to have attributes first, then methods - not attribute, method, attribute, method etc.

Let's limit the changed to snowpark only. It's easier to revert or review when changes has well defined scope.

sfc-gh-turbaszek · 2024-08-02T07:41:14Z

src/snowflake/cli/api/project/schemas/project_definition.py

+    def validate_entities(
+        cls, entities: Dict[str, AnnotatedEntity]
+    ) -> Dict[str, AnnotatedEntity]:
        for key, entity in entities.items():
            # TODO Automatically detect TargetFields to validate
-            if entity.type == ApplicationEntity.get_type():
-                if isinstance(entity.from_, TargetField):
-                    target_key = entity.from_.target
-                    target_object = entity.from_
-                    target_type = target_object.get_type()
-                    cls._validate_target_field(target_key, target_type, entities)
+            if isinstance(entity, list):
+                for e in entity:
+                    cls._validate_single_entity(e, entities)
+            else:
+                cls._validate_single_entity(entity, entities)
        return entities

+    @classmethod
+    def _validate_single_entity(
+        cls, entity: Entity, entities: Dict[str, AnnotatedEntity]
+    ):
+        if entity.type == ApplicationEntity.get_type():
+            if isinstance(entity.from_, TargetField):
+                target_key = entity.from_.target
+                target_object = entity.from_
+                target_type = target_object.get_type()
+                cls._validate_target_field(target_key, target_type, entities)
+


How this change relates to snowpark?

Again - it was moved in file, to make the code more readable

sfc-gh-turbaszek · 2024-08-02T07:45:47Z

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

+    stage_names = {
+        entity.stage for entity in [*functions.values(), *procedures.values()]
+    }
+    stage_manager = StageManager()
+    project_name = (
+        pd.defaults.project_name if pd.defaults.project_name else "my_snowpark_project"
+    )
+    for stage in stage_names:
+        stage = FQN.from_string(stage).using_context()
+        stage_manager.create(
+            stage_name=stage, comment="deployments managed by Snowflake CLI"
+        )
+        artifact_stage_directory = get_app_stage_path(stage, project_name)


We create multiple stages but use only one artifact_stage_directory? I think we may want to consider doing

for stage in stages: create_stage(stage) for udf_sproc in stage_to_objects_map[stage]: _deploy_single_object()

WDYT? In this way we are processing stage by stage. In case of single stage there's no difference compared to current bahavior.

In what way? Currently new artifact_stage_directory is created for each stage.
We use common project name, as we can have multiple udfs on single stage, and need a name to group them.

This loop:

for stage in stage_names: stage = FQN.from_string(stage).using_context() stage_manager.create( stage_name=stage, comment="deployments managed by Snowflake CLI" ) artifact_stage_directory = get_app_stage_path(stage, project_name)

Results in artifact_stage_directory being directory for last stage in stage_names. And this single value is used for all entities independent of their stages.

Corrected - now it's all in a loop.

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

sfc-gh-turbaszek · 2024-08-02T07:47:37Z

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

    )
-    log.info("Building package using sources from: %s", snowpark_paths.source.path)
+    log.info("Building package using sources from:")
+    log.info(",".join(str(s) for s in snowpark_paths.sources))


Instead of that duplication check, move project_definition to signature.

I'm not sure if i understand

cli_context = get_cli_context() pd = cli_context.project_definition if not pd.meets_version_requirement("2"): pd = _migrate_v1_snowpark_to_v2(pd)`

this duplicated code, we did the check in very first line in command definition. Consider making:

def _deploy_single_object( cli_contex, <-- here manager: FunctionManager | ProcedureManager, object_type: ObjectType, object_definition: SnowparkEntity, existing_objects: Dict[str, Dict], snowflake_dependencies: List[str], stage_artifact_path: str, ):

I moved it to a helper function, as it is used in bot build and deploy commands

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

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

Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>

Co-authored-by: Patryk Czajka <patryk.czajka@snowflake.com>

sfc-gh-turbaszek · 2024-08-06T12:46:08Z

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

+        default=[],
+    )
+    stage: str = Field(title="Stage in which artifacts will be stored")
+    src: str = Field(title="Folder where your code should be located")


source and imports should be replaced with artifacts as per design

But it seems, that artifacts mean local files that will be uploaded - so imports should be separated, as those are the files that already exist server-side, and just need to be included in the SQL declaration

sfc-gh-gbloom · 2024-08-06T16:18:15Z

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

+    }
+    stage_manager = StageManager()
+    project_name = (
+        pd.defaults.project_name if pd.defaults.project_name else "my_snowpark_project"


Should we put the default "my_snowpark_project" value under SnowparkEntity?
Also, each defaults entry will automatically apply to each entity that has this field but was not specified by the user. Not sure this check is needed here.

# Conflicts: # src/snowflake/cli/_plugins/snowpark/commands.py

# Conflicts: # src/snowflake/cli/api/project/schemas/entities/entities.py # src/snowflake/cli/api/project/schemas/project_definition.py

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

sfc-gh-gbloom · 2024-08-08T12:56:37Z

src/snowflake/cli/api/entities/snowpark_entity.py

+class SnowparkEntity(EntityBase):
+    pass
+
+
+class FunctionEntity(SnowparkEntity):
+    pass
+
+
+class ProcedureEntity(SnowparkEntity):
+    pass


Should all 3 be in the same file?

They`re so closely related, it seems logical

We can refactor later if we need to, but I imagine we would want to compose functions and procedures with other non-snowpark entities (maybe even as standalone?)

sfc-gh-gbloom · 2024-08-08T13:00:19Z

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

+    for stage in stage_names:
+        stage = FQN.from_string(stage).using_context()
+        stage_manager.create(fqn=stage, comment="deployments managed by Snowflake CLI")
+        artifact_stage_directory = get_app_stage_path(stage, pd.defaults.project_name)


You're assuming pd.defaults.project_name is defined, but shouldn't it be optional? What should be the stage if different values are defined on each entity?

ATM we have only one snowpark project value. I think we should introduce different stage paths after introducing mixins, as it will change much more logic. I.e. now we create only one artifact for all snowpark entities.

I now realize you used the defaults section to store global-like values for entities to reference, but it's not exactly its purpose. You can think of defaults as automatic mixins that are applied to all entities if they have a value of the same name and the user didn't specify it on the entity.

In this example sp1.project_name will be "proj1", and sp2.project_name will be "default_proj":

entities: sp1: type: snowpark project_name: proj1 sp2: type: snowpark defaults: project_name: default_proj

Yes, you're right- i used the default value when converting from v1 and went on with this logic.
Nevertheless - currently it doesn't change anything, as we use single artifact for all snowpark entities

sfc-gh-gbloom · 2024-08-08T15:08:35Z

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

+from snowflake.cli.api.project.schemas.updatable_model import DiscriminatorField
+
+
+class SnowparkEntityModel(EntityModelBase):


Aren't we missing an entity type here?

Suggested change

class SnowparkEntityModel(EntityModelBase):

class SnowparkEntityModel(EntityModelBase):

type: Literal["snowpark"] = DiscriminatorField() # noqa: A003

In what context snowpark would be used? Currently it's a base class for common elements between entities.

Got it. For clarity, maybe we should have SnowparkEntityModel to also inherit from ABC and add "Base" to its name?
SnowparkFunctionEntity might also be more accurate.

sfc-gh-gbloom · 2024-08-08T15:12:47Z

src/snowflake/cli/api/entities/snowpark_entity.py

+class SnowparkEntity(EntityBase):
+    pass
+
+
+class FunctionEntity(SnowparkEntity):
+    pass
+
+
+class ProcedureEntity(SnowparkEntity):
+    pass


We can refactor later if we need to, but I imagine we would want to compose functions and procedures with other non-snowpark entities (maybe even as standalone?)

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

sfc-gh-gbloom · 2024-08-08T15:14:02Z

tests/project/test_project_definition_v2.py

+                "defaults": {"stage": "dev"},
+                "entities": {
+                    "function1": {
+                        "type": "function",
+                        "name": "name",
+                        "handler": "app.hello",
+                        "returns": "string",
+                        "signature": [{"name": "name", "type": "string"}],
+                        "runtime": "3.8",
+                        "artifacts": "src",
+                    }
+                },


Shouldn't we also have a test for "type": "procedure"?

Good point, added

sfc-gh-gbloom · 2024-08-08T15:19:36Z

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

+    for stage in stage_names:
+        stage = FQN.from_string(stage).using_context()
+        stage_manager.create(fqn=stage, comment="deployments managed by Snowflake CLI")
+        artifact_stage_directory = get_app_stage_path(stage, pd.defaults.project_name)


I now realize you used the defaults section to store global-like values for entities to reference, but it's not exactly its purpose. You can think of defaults as automatic mixins that are applied to all entities if they have a value of the same name and the user didn't specify it on the entity.

In this example sp1.project_name will be "proj1", and sp2.project_name will be "default_proj":

entities: sp1: type: snowpark project_name: proj1 sp2: type: snowpark defaults: project_name: default_proj

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

# Conflicts: # src/snowflake/cli/_plugins/snowpark/common.py

sfc-gh-turbaszek and others added 8 commits July 26, 2024 14:07

Support project definition V2 in streamlit deploy command (#1369)

8f9b89a

Init

1603dd8

Solution outline

06c6d63

Merge branch 'main' into jsikorski/snowpark-v2

45703d9

# Conflicts: # src/snowflake/cli/plugins/snowpark/commands.py # src/snowflake/cli/plugins/streamlit/commands.py # src/snowflake/cli/templates/default_snowpark/snowflake.yml # src/snowflake/cli/templates/default_streamlit/snowflake.yml

Fixing paths

0884c45

build fix

55761e9

build fix

f226451

sfc-gh-turbaszek reviewed Aug 1, 2024

View reviewed changes

src/snowflake/cli/api/project/schemas/entities/snowpark_entity.py Outdated Show resolved Hide resolved

sfc-gh-turbaszek reviewed Aug 1, 2024

View reviewed changes

src/snowflake/cli/api/project/schemas/entities/snowpark_entity.py Outdated Show resolved Hide resolved

sfc-gh-jsikorski and others added 2 commits August 1, 2024 11:17

Update src/snowflake/cli/api/project/schemas/entities/snowpark_entity.py

ac53ecc

Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>

Update src/snowflake/cli/api/project/schemas/entities/snowpark_entity.py

10fc53f

Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>

sfc-gh-turbaszek reviewed Aug 1, 2024

View reviewed changes

sfc-gh-jsikorski added 5 commits August 1, 2024 16:38

query problem

a3db93b

Merge branch 'main' into jsikorski/snowpark-v2

cf933f2

Fix for zipper

563886d

Test fix

5a4a7a0

Test fix

403e3c7

sfc-gh-jsikorski marked this pull request as ready for review August 2, 2024 05:48

sfc-gh-jsikorski requested review from a team as code owners August 2, 2024 05:48

sfc-gh-turbaszek reviewed Aug 2, 2024

View reviewed changes

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

sfc-gh-turbaszek reviewed Aug 2, 2024

View reviewed changes

sfc-gh-pczajka reviewed Aug 2, 2024

View reviewed changes

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

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

src/snowflake/cli/plugins/snowpark/common.py Outdated Show resolved Hide resolved

sfc-gh-jsikorski and others added 3 commits August 2, 2024 12:27

Update src/snowflake/cli/plugins/snowpark/commands.py

67710b8

Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>

Update src/snowflake/cli/plugins/snowpark/commands.py

01c284e

Co-authored-by: Patryk Czajka <patryk.czajka@snowflake.com>

Merge branch 'main' into jsikorski/snowpark-v2

8553cf7

sfc-gh-turbaszek reviewed Aug 6, 2024

View reviewed changes

sfc-gh-gbloom reviewed Aug 6, 2024

View reviewed changes

sfc-gh-jsikorski and others added 8 commits August 6, 2024 19:01

Merge branch 'main' into jsikorski/snowpark-v2

0747807

# Conflicts: # src/snowflake/cli/_plugins/snowpark/commands.py

Merge cleanup

a7d449a

Fixup

2766b72

Merge branch 'main' into jsikorski/snowpark-v2

659636f

Changed to artifacts

be0d58f

Merge branch 'main' into jsikorski/snowpark-v2

221d5e5

# Conflicts: # src/snowflake/cli/api/project/schemas/entities/entities.py # src/snowflake/cli/api/project/schemas/project_definition.py

Fixup

4176171

Merge branch 'main' into jsikorski/snowpark-v2

bff4b4d

sfc-gh-jsikorski requested review from sfc-gh-turbaszek, sfc-gh-gbloom and a team August 8, 2024 11:54

sfc-gh-jsikorski enabled auto-merge (squash) August 8, 2024 12:00

sfc-gh-gbloom reviewed Aug 8, 2024

View reviewed changes

sfc-gh-jsikorski requested a review from sfc-gh-gbloom August 8, 2024 14:34

Fixup

33ac466

sfc-gh-gbloom reviewed Aug 8, 2024

View reviewed changes

sfc-gh-jsikorski added 3 commits August 9, 2024 13:22

After review fixes

1f6d50c

Merge branch 'main' into jsikorski/snowpark-v2

ccdea97

# Conflicts: # src/snowflake/cli/_plugins/snowpark/common.py

Post merge cleanup

bd98653

sfc-gh-jsikorski requested a review from sfc-gh-gbloom August 9, 2024 11:42

sfc-gh-gbloom approved these changes Aug 9, 2024

View reviewed changes

sfc-gh-turbaszek approved these changes Aug 10, 2024

View reviewed changes

Merge branch 'main' into jsikorski/snowpark-v2

91b7a5f

sfc-gh-jsikorski merged commit bb4ee2b into main Aug 10, 2024
16 checks passed

sfc-gh-jsikorski deleted the jsikorski/snowpark-v2 branch August 10, 2024 06:50

		from snowflake.cli.api.project.schemas.updatable_model import DiscriminatorField


		class SnowparkEntityModel(EntityModelBase):

	class SnowparkEntityModel(EntityModelBase):
	class SnowparkEntityModel(EntityModelBase):
	type: Literal["snowpark"] = DiscriminatorField() # noqa: A003

V2.0 Definition for snowpark #1402

V2.0 Definition for snowpark #1402

Conversation

sfc-gh-jsikorski commented Aug 1, 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

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-jsikorski commented Aug 1, 2024 •

edited

Loading