-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mock snowpark entities #1957
Mock snowpark entities #1957
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,100 @@ | ||
from typing import Generic, TypeVar | ||
import os | ||
from pathlib import Path | ||
from typing import Generic, Optional, TypeVar | ||
|
||
from snowflake.cli._plugins.nativeapp.artifacts import build_bundle | ||
from snowflake.cli._plugins.nativeapp.entities.application_package_child_interface import ( | ||
ApplicationPackageChildInterface, | ||
) | ||
from snowflake.cli._plugins.nativeapp.feature_flags import FeatureFlag | ||
from snowflake.cli._plugins.snowpark.snowpark_entity_model import ( | ||
FunctionEntityModel, | ||
ProcedureEntityModel, | ||
) | ||
from snowflake.cli.api.entities.common import EntityBase | ||
from snowflake.cli.api.project.schemas.v1.native_app.path_mapping import PathMapping | ||
|
||
T = TypeVar("T") | ||
|
||
|
||
class SnowparkEntity(EntityBase[Generic[T]]): | ||
pass | ||
# WARNING: The Function/Procedure entities are not implemented yet. The logic below is only for demonstrating the | ||
# required interfaces for composability (used by ApplicationPackageEntity behind a feature flag). | ||
class SnowparkEntity(EntityBase[Generic[T]], ApplicationPackageChildInterface): | ||
def __init__(self, *args, **kwargs): | ||
if not FeatureFlag.ENABLE_NATIVE_APP_CHILDREN.is_enabled(): | ||
raise NotImplementedError("Snowpark entities are not implemented yet") | ||
super().__init__(*args, **kwargs) | ||
|
||
@property | ||
def project_root(self) -> Path: | ||
return self._workspace_ctx.project_root | ||
|
||
@property | ||
def deploy_root(self) -> Path: | ||
return self.project_root / "output" / "deploy" | ||
|
||
def action_bundle( | ||
self, | ||
*args, | ||
**kwargs, | ||
): | ||
return self.bundle() | ||
|
||
def bundle(self, bundle_root=None): | ||
return build_bundle( | ||
self.project_root, | ||
bundle_root or self.deploy_root, | ||
[ | ||
PathMapping(src=str(artifact.src), dest=artifact.dest) | ||
for artifact in self._entity_model.artifacts | ||
], | ||
) | ||
|
||
def _get_identifier_for_sql( | ||
self, arg_names: bool = True, schema: Optional[str] = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need lots of comments for this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a simplified version of |
||
) -> str: | ||
model = self._entity_model | ||
if arg_names: | ||
signature = ", ".join( | ||
f"{arg.name} {arg.arg_type}" for arg in model.signature | ||
) | ||
else: | ||
signature = ", ".join(arg.arg_type for arg in model.signature) | ||
entity_id = self.entity_id | ||
object_name = f"{schema}.{entity_id}" if schema else entity_id | ||
return f"{object_name}({signature})" | ||
|
||
def get_deploy_sql( | ||
self, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add docstring on public methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstrings are on the interface class instead of on each entity |
||
artifacts_dir: Optional[Path] = None, | ||
schema: Optional[str] = None, | ||
): | ||
model = self._entity_model | ||
imports = [f"'{x}'" for x in model.imports] | ||
if artifacts_dir: | ||
for root, _, files in os.walk(self.deploy_root / artifacts_dir): | ||
for f in files: | ||
file_path_relative_to_deploy_root = ( | ||
Path(root).relative_to(self.deploy_root) / f | ||
) | ||
imports.append(f"'/{str(file_path_relative_to_deploy_root)}'") | ||
|
||
entity_type = model.get_type().upper() | ||
|
||
query = [ | ||
f"CREATE OR REPLACE {entity_type} {self._get_identifier_for_sql(schema=schema)}", | ||
f"RETURNS {model.returns}", | ||
"LANGUAGE python", | ||
"RUNTIME_VERSION=3.8", | ||
f"IMPORTS=({', '.join(imports)})", | ||
f"HANDLER='{model.handler}'", | ||
"PACKAGES=('snowflake-snowpark-python');", | ||
] | ||
return "\n".join(query) | ||
|
||
def get_usage_grant_sql(self, app_role: str, schema: Optional[str] = None): | ||
entity_type = self._entity_model.get_type().upper() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably use to_identifier() to be safe:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These args are already called with |
||
return f"GRANT USAGE ON {entity_type} {self._get_identifier_for_sql(schema=schema, arg_names=False)} TO APPLICATION ROLE {app_role};" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have a mapping instead. It's a bit coincidental that the internal entity type matches SQL type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I agree, but the entire logic of these methods is just a placeholder until CLI team will replace it with the real entity implementation very soon. It's not intended to be production ready, as many other features are missing. The main goal of this PR is to add the contract between snowpark/application-package for the children integration. |
||
|
||
|
||
class FunctionEntity(SnowparkEntity[FunctionEntityModel]): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
from __future__ import annotations | ||
|
||
from pathlib import Path | ||
from textwrap import dedent | ||
|
||
import pytest | ||
from snowflake.cli._plugins.snowpark.snowpark_entity import ( | ||
FunctionEntity, | ||
ProcedureEntity, | ||
) | ||
from snowflake.cli._plugins.snowpark.snowpark_entity_model import ( | ||
FunctionEntityModel, | ||
ProcedureEntityModel, | ||
) | ||
from snowflake.cli._plugins.workspace.context import WorkspaceContext | ||
from snowflake.cli.api.console import cli_console as cc | ||
from snowflake.cli.api.project.definition_manager import DefinitionManager | ||
|
||
from tests.testing_utils.mock_config import mock_config_key | ||
|
||
|
||
def test_cannot_instantiate_without_feature_flag(): | ||
with pytest.raises(NotImplementedError) as err: | ||
FunctionEntity() | ||
assert str(err.value) == "Snowpark entities are not implemented yet" | ||
|
||
with pytest.raises(NotImplementedError) as err: | ||
ProcedureEntity() | ||
assert str(err.value) == "Snowpark entities are not implemented yet" | ||
|
||
|
||
def test_function_implements_nativeapp_children_interface(temp_dir): | ||
with mock_config_key("enable_native_app_children", True): | ||
dm = DefinitionManager() | ||
ctx = WorkspaceContext( | ||
console=cc, | ||
project_root=dm.project_root, | ||
get_default_role=lambda: "mock_role", | ||
get_default_warehouse=lambda: "mock_warehouse", | ||
) | ||
main_file = "main.py" | ||
(Path(temp_dir) / main_file).touch() | ||
model = FunctionEntityModel( | ||
type="function", | ||
handler="my_schema.my_func", | ||
returns="integer", | ||
signature=[ | ||
{"name": "input_number", "type": "integer"}, | ||
{"name": "input_string", "type": "text"}, | ||
], | ||
stage="my_stage", | ||
artifacts=[main_file], | ||
) | ||
model._entity_id = "my_func" # noqa: SLF001 | ||
schema = "my_schema" | ||
fn = FunctionEntity(model, ctx) | ||
|
||
fn.bundle() | ||
bundle_artifact = Path(temp_dir) / "output" / "deploy" / main_file | ||
deploy_sql_str = fn.get_deploy_sql(schema=schema) | ||
grant_sql_str = fn.get_usage_grant_sql(app_role="app_role", schema=schema) | ||
|
||
assert bundle_artifact.exists() | ||
assert ( | ||
deploy_sql_str | ||
== dedent( | ||
""" | ||
CREATE OR REPLACE FUNCTION my_schema.my_func(input_number integer, input_string text) | ||
RETURNS integer | ||
LANGUAGE python | ||
RUNTIME_VERSION=3.8 | ||
IMPORTS=() | ||
HANDLER='my_schema.my_func' | ||
PACKAGES=('snowflake-snowpark-python'); | ||
""" | ||
).strip() | ||
) | ||
assert ( | ||
grant_sql_str | ||
== "GRANT USAGE ON FUNCTION my_schema.my_func(integer, text) TO APPLICATION ROLE app_role;" | ||
) | ||
|
||
|
||
def test_procedure_implements_nativeapp_children_interface(temp_dir): | ||
with mock_config_key("enable_native_app_children", True): | ||
dm = DefinitionManager() | ||
ctx = WorkspaceContext( | ||
console=cc, | ||
project_root=dm.project_root, | ||
get_default_role=lambda: "mock_role", | ||
get_default_warehouse=lambda: "mock_warehouse", | ||
) | ||
main_file = "main.py" | ||
(Path(temp_dir) / main_file).touch() | ||
model = ProcedureEntityModel( | ||
type="procedure", | ||
handler="my_schema.my_sproc", | ||
returns="integer", | ||
signature=[ | ||
{"name": "input_number", "type": "integer"}, | ||
{"name": "input_string", "type": "text"}, | ||
], | ||
stage="my_stage", | ||
artifacts=[main_file], | ||
) | ||
model._entity_id = "my_sproc" # noqa: SLF001 | ||
schema = "my_schema" | ||
fn = ProcedureEntity(model, ctx) | ||
|
||
fn.bundle() | ||
bundle_artifact = Path(temp_dir) / "output" / "deploy" / main_file | ||
deploy_sql_str = fn.get_deploy_sql(schema=schema) | ||
grant_sql_str = fn.get_usage_grant_sql(app_role="app_role", schema=schema) | ||
|
||
assert bundle_artifact.exists() | ||
assert ( | ||
deploy_sql_str | ||
== dedent( | ||
""" | ||
CREATE OR REPLACE PROCEDURE my_schema.my_sproc(input_number integer, input_string text) | ||
RETURNS integer | ||
LANGUAGE python | ||
RUNTIME_VERSION=3.8 | ||
IMPORTS=() | ||
HANDLER='my_schema.my_sproc' | ||
PACKAGES=('snowflake-snowpark-python'); | ||
""" | ||
).strip() | ||
) | ||
assert ( | ||
grant_sql_str | ||
== "GRANT USAGE ON PROCEDURE my_schema.my_sproc(integer, text) TO APPLICATION ROLE app_role;" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to simplify the inheritence here, maybe by using aggregation instead?
You don't have to change it if required, but I am just wondering about the reason we have it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using inheritance as an "implements interface", there is no real inheritance here. But I'm open for suggestions for a more elegant way -- can you give an example how it can be simplified?