Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mock snowpark entities #1957

Closed
wants to merge 4 commits into from
Closed

Mock snowpark entities #1957

wants to merge 4 commits into from

Conversation

sfc-gh-gbloom
Copy link
Contributor

@sfc-gh-gbloom sfc-gh-gbloom commented Dec 18, 2024 •
edited
Loading

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

Same as #1856, adding mock snowpark entities as well to be used as children of ApplicationPackageEntity.

Manual test

Cloned the mail-order native app example, converted snowflake.yml to v2 and extracted a single sproc to an entity:

definition_version: '2'
entities:
  pkg:
    identifier: mailorder_pkg_gbloom
    type: application package
    artifacts:
    - src: app/*
      dest: ./
    - src: python/src/*
      dest: ./python/
    stage: app_src.stage
    children:
      - target: record_tax_transfer
        identifier:
          schema: "regions"
        ensure_usable_by:
          application_roles: ["app_csr", "app_admin"]

  app:
    identifier: mailorder_gbloom
    type: application
    from:
      target: pkg

  record_tax_transfer:
    type: procedure
    identifier:
      name: record_tax_transfer
    handler: regions.record_tax_transfer
    signature:
      - name: region_id
        type: text
      - name: amount_cents
        type: integer
    returns: integer
    artifacts:
      - dest: py/
        src: python/src/regions.py
    stage: my_stage

Running snow app bundle created the correct bundle structure:

output/deploy
├── README.md
├── manifest.yml
├── setup.sql
├── _children
│   └── record_tax_transfer
│       └── py
│           └── regions.py
├── data ...
├── ledger ...
├── python ...
├── receipts ...
├── regions ...
└── ui ...

And the correct setup script section:

-- AUTO GENERATED CHILDREN SECTION
CREATE OR REPLACE PROCEDURE regions.record_tax_transfer(region_id text, amount_cents integer)
RETURNS integer
LANGUAGE python
RUNTIME_VERSION=3.8
IMPORTS=('/_children/record_tax_transfer/py/regions.py')
HANDLER='regions.record_tax_transfer'
PACKAGES=('snowflake-snowpark-python');
CREATE APPLICATION ROLE IF NOT EXISTS app_csr;
GRANT USAGE ON SCHEMA regions TO APPLICATION ROLE app_csr;
GRANT USAGE ON PROCEDURE regions.record_tax_transfer(text, integer) TO APPLICATION ROLE app_csr;

Running snow app run deployed the app successfully, and the streamlit was able to use the sproc.

@sfc-gh-gbloom sfc-gh-gbloom requested a review from a team as a code owner December 18, 2024 22:21
sfc-gh-melnacouzi
sfc-gh-melnacouzi reviewed Dec 19, 2024
View reviewed changes
src/snowflake/cli/_plugins/snowpark/snowpark_entity.py
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):
Copy link
Contributor

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?

Copy link
Contributor Author

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?

src/snowflake/cli/_plugins/snowpark/snowpark_entity.py
object_name = f"{schema}.{entity_id}" if schema else entity_id
return f"{object_name}({signature})"

def get_deploy_sql(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add docstring on public methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstrings are on the interface class instead of on each entity

src/snowflake/cli/_plugins/snowpark/snowpark_entity.py
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()
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

src/snowflake/cli/_plugins/snowpark/snowpark_entity.py
]
return "\n".join(query)

def get_usage_grant_sql(self, app_role: str, schema: Optional[str] = None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably use to_identifier() to be safe:

app_role = to_identifier(app_role)
schema = to_identifier(schema)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These args are already called with to_identifier() when get_usage_grant_sql() is called from - link

src/snowflake/cli/_plugins/snowpark/snowpark_entity.py
],
)

def _get_identifier_for_sql(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need lots of comments for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a simplified version of identifier_for_sql() -- again, just as a placeholder logic that will be replaced soon

@sfc-gh-jsikorski
Copy link
Contributor

This duplicates #1959 - so my proposition is to copy from this PR anything that is missing in the other and close this one.

@sfc-gh-gbloom
Copy link
Contributor Author

This duplicates #1959 - so my proposition is to copy from this PR anything that is missing in the other and close this one.

@sfc-gh-jsikorski Oh I wasn't aware of the other PR! Closing this PR, feel free to use it as a reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-melnacouzi sfc-gh-melnacouzi sfc-gh-melnacouzi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-gbloom @sfc-gh-jsikorski @sfc-gh-melnacouzi