Switch from StrictYAML to Pydantic #870

sfc-gh-jsikorski · 2024-03-05T15:52:44Z

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

Switch to Pydantic from StrictYaml. No we use Models for project definition files validation. This gives us:

IDE-supported, safe access to definition fields
Better control on types, names etc.
Stricter control over input files
No need to use hardcoded strings
Ability to generate JSON schemas and docs from code

sfc-gh-jsikorski · 2024-03-05T15:57:02Z

tests/nativeapp/test_run_processor.py

@@ -794,6 +794,7 @@ def test_create_dev_app_create_new_quoted(
                - src: app/streamlit/*.py
                dest: ui/

+


Is this dest:ui/ placed here intentionally? It's an extra value, that is not used in schema? According to design doc, we should not accept extra value, so this causes an error. Should it be deleted?

@sfc-gh-bgoel @sfc-gh-cgorrie can you advise?

I think this is an indentation error -- this is part of the last entry of the artifacts collection (i.e. { src: "app/streamlit/*.py", dest: "ui/" }).

RELEASE-NOTES.md

src/snowflake/cli/api/commands/flags.py

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

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

sfc-gh-turbaszek · 2024-03-06T08:29:56Z

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

+    )
+    streamlit: Optional[StreamlitSchema] = Field(
+        title="Native app definitions for the project", default=None
+    )


Let's add validator for definition_version

you mean definition == 1?

Another way to think about this -- when we get to project definition v2 (design currently in progress) it will look completely different. We should have a way to separate out the definitions by definition_version rather than have to have a complex base model that has to support both. I agree that for now ensuring definition_version == 1 is important, but we should make sure that when we introduce definition_version == 2 it's something that can live alongside this model rather than "within" it, if we have enough changes that it makes sense.

Ok, so my proposition is to add definition_version==1 to field validation. If user provides another version of definition, it's pointless to proceed with parsing any fields.
Then, when we add new versions, we can change load_project_definition() logic to decide which model should be used for provided data.

I agree, let's have ==1 for now. We will add more validation once we start working on v2.

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

sfc-gh-turbaszek · 2024-03-06T08:32:13Z

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

 DB_SCHEMA_AND_NAME = f"{IDENTIFIER}[.]{IDENTIFIER}[.]{IDENTIFIER}"
-SCHEMA_AND_NAME = f"{IDENTIFIER}[.]{IDENTIFIER}"
+SCHEMA_AND_NAME = f"{IDENTIFIER}[.]{IDENTIFIER_NO_LENGTH}"


Can you explain this change?

That was a lost artifact of long battle with regexes. Long story.

src/snowflake/cli/plugins/nativeapp/manager.py

sfc-gh-turbaszek · 2024-03-06T08:36:12Z

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

-        query_warehouse=streamlit["query_warehouse"],
-        additional_source_files=streamlit.get("additional_source_files"),
+        query_warehouse=streamlit.query_warehouse,
+        additional_source_files=streamlit.additional_source_files,


I love this change, getting rid of all those hardcoded strings rocks 🪨

tests/project/test_config.py

sfc-gh-turbaszek · 2024-03-06T08:38:49Z

tests/project/test_config.py

+    assert (
+        "Field required [type=missing, input_value={'name': 'underspecified'}, input_type=dict]"
+        in str(exc_info.value)
+    )


This information should be as useful as the previous one. In current version I don't know if artifacts are the problem

Implemented

tests/project/test_config.py

This reverts commit 80467b3. mraba/app-factory: create app with factory (#860) * mraba/app-factory: create app with factory SNOW-1043081: Adding support for qualified names for image repositories. (#823) * SNOW-1043081: Adding support for qualified image repository names * SNOW-1043081: Fixing test imports * SNOW-1043081: Adding tests for getting image repository url without db or schema SNOW-1011771: Added generic REPLACE, IF EXISTS, IF NOT EXISTS flags (#826) * SNOW-1011771: Adding generic OR REPLACE, IF EXISTS, IF NOT EXISTS flags to flags.py * SNOW-1011771: Using generic ReplaceOption in snowpark deploy and streamlit deploy * SNOW-1011771: Using generic IfNotExistsOption in compute pool create and updating unit tests. * SNOW-1011771: Using generic IfNotExistsOption in service create and updating unit tests * SNOW-1011771: Using generic ReplaceOption and IfNotExistsOption in image-repository create. * SNOW-1011771: Fixup * SNOW-1011771: Update release notes * SNOW-1011771: Update test_help_messages * SNOW-1011771: precommit * SNOW-1011771: Adding validation that only one create mode option can be set at once * fixup * SNOW-1011771: Updating tests for REPLACE AND IF NOT EXISTS case on image-repository create to throw error * SNOW-1011771: Adding snapshots * SNOW-1011771: Adding a new mutually_exclusive field to OverrideableOption * formatting * SNOW-1011771: Adding tests for OverrideableOption * SNOW-1011771: Fixing test failures due to improperly quoted string Add snow --help to test_help_messages (#821) * Add snow --help to test_help_messages * update snapshot Avoid plain print, make sure silent is eager flag (#871) [NADE] Update CODEOWNERS to use NADE team id. (#873) update to using nade team in codeowners New workflow to stop running workflows after new commit (#862) * new workflow * new workflow * new workflow * new workflow * typo fix * typo fix * import fix * import fix * import fix * import fix * import fix * import fix * import fix * new approach * new approach * new approach * new approach * new approach * New approach * added to test * Added to more workflows * Dummy commit Schemas adjusting native apps to streamlit fixing streamlit fixies after unit tests fixies after unit tests fixing for snowflake fixing for snowflake Fixes after review Fixes after review Fixes after review

sfc-gh-cgorrie · 2024-03-06T16:19:11Z

@sfc-gh-jsikorski just thinking a bit ahead to IDE integration. It looks like pydantic has the ability to generate JSON schema, which we'd like to consume inside of IDEs. Eventually we should extract the project definition schemas into their own open-source project which can also host these JSON schemas as part of a CD process.

Wanted to confirm this medium-term vision is compatible with your future plans too!

sfc-gh-cgorrie

Some initial questions and comments while I dive deeper into the code.

sfc-gh-cgorrie · 2024-03-06T16:19:51Z

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

@@ -100,5 +101,5 @@ def _user_definition_file_if_available(project_path: Path) -> Optional[Path]:
        )

    @functools.cached_property
-    def project_definition(self) -> dict:
+    def project_definition(self) -> ProjectDefinition:


Love the type safety Pydantic gives us!

Me too :)
As i cannot answer the comment above - yes, Pydantic can be used to generate JSON schemas, but i think i'll summon @sfc-gh-turbaszek to answer the long-term vision plans :D

And we could use description and examples fields of FieldInfo, to have the schema well documented.

sfc-gh-cgorrie · 2024-03-06T16:22:37Z

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

+    )
+    streamlit: Optional[StreamlitSchema] = Field(
+        title="Native app definitions for the project", default=None
+    )


Another way to think about this -- when we get to project definition v2 (design currently in progress) it will look completely different. We should have a way to separate out the definitions by definition_version rather than have to have a complex base model that has to support both. I agree that for now ensuring definition_version == 1 is important, but we should make sure that when we introduce definition_version == 2 it's something that can live alongside this model rather than "within" it, if we have enough changes that it makes sense.

src/snowflake/cli/api/project/schemas/native_app/path_maping.py

sfc-gh-cgorrie · 2024-03-06T16:25:28Z

tests/nativeapp/test_run_processor.py

@@ -794,6 +794,7 @@ def test_create_dev_app_create_new_quoted(
                - src: app/streamlit/*.py
                dest: ui/

+


I think this is an indentation error -- this is part of the last entry of the artifacts collection (i.e. { src: "app/streamlit/*.py", dest: "ui/" }).

sfc-gh-cgorrie

Some more comments and suggestions.

Also I have a question, is it intentional that we have a test_pydantic_schemas.py that is empty?

sfc-gh-cgorrie · 2024-03-06T16:50:06Z

RELEASE-NOTES.md

@@ -10,6 +10,8 @@
 ## Fixes and improvements
 * Adding `--image-name` option for image name argument in `spcs image-repository list-tags` for consistency with other commands.
 * Fixed errors during `spcs image-registry login` not being formatted correctly.
+* Project definition no longer accept extra fields. Any extra field will cause an error.
+* Project definition no longer accept extra fields. Any extra field will cause an error.


Duplicate line

src/snowflake/cli/api/project/schemas/native_app/package.py

sfc-gh-turbaszek · 2024-03-07T11:11:05Z

@sfc-gh-jsikorski just thinking a bit ahead to IDE integration. It looks like pydantic has the ability to generate JSON schema, which we'd like to consume inside of IDEs. Eventually we should extract the project definition schemas into their own open-source project which can also host these JSON schemas as part of a CD process.

Wanted to confirm this medium-term vision is compatible with your future plans too!

The ability to generate JSON schema is one of the main goals (IDE, docs, etc). In terms of vision - extracting this to separate package is not planned yet, but once we have py ecosystem then we can easily do so.

sfc-gh-turbaszek · 2024-03-07T11:43:13Z

pyproject.toml

@@ -25,6 +25,7 @@ dependencies = [
  "typer==0.9.0",
  "urllib3>=1.21.1,<2.3",
  "GitPython==3.1.42",
+  "pydantic==2.6.3"


Should we remove need for strictyml? Or should it be done in separate PR?

Un-done. Left pydantic for some uses not directly related to project definitions

sfc-gh-bgoel

The main changes LGTM. Left some comments about messages shown to the user which need to be in accordance with our PMM team's guidelines.

sfc-gh-bgoel · 2024-03-08T13:56:19Z

src/snowflake/cli/api/project/schemas/native_app/application.py

+
+class Application(UpdatableModel):
+    role: Optional[str] = Field(
+        title="Role to use when creating the application instance and consumer-side objects",


Nit: "application object" instead of "application instance"

sfc-gh-bgoel · 2024-03-08T13:56:37Z

src/snowflake/cli/api/project/schemas/native_app/application.py

+        default=None,
+    )
+    name: Optional[str] = Field(
+        title="Name of the application created when you run the snow app run command",


"... application object created ..."

sfc-gh-bgoel · 2024-03-08T13:57:04Z

src/snowflake/cli/api/project/schemas/native_app/application.py

+        default=None,
+    )
+    warehouse: Optional[str] = IdentifierField(
+        title="Name of the application created when you run the snow app run command",


ditto, and for line 26 as well.

sfc-gh-bgoel · 2024-03-08T13:58:37Z

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

+    @classmethod
+    def validate_source_stage(cls, input_value: str):
+        if not re.match(SCHEMA_AND_NAME, input_value):
+            raise ValueError("Incorrect value for Native Apps source stage value")


Maybe we can rephrase to:
Incorrect value for source_stage property of native_app.
The goal is to avoid using "Native Apps" as a term, which comes from our marketing teams.

+1, I think latter one we can add something like regex_validator(value, pattern) that would cause a generic error like Incorrect value for native_app.source_stage. Allowed values: pattern.

sfc-gh-bgoel

LGTM for native app and project changes, thank you!

sfc-gh-bgoel

re-approving :)

* Revert "Bump tomlkit from 0.12.3 to 0.12.4 (#848)" (#863) This reverts commit a3ed7cc. mraba/app-factory: create app with factory (#860) * mraba/app-factory: create app with factory SNOW-1043081: Adding support for qualified names for image repositories. (#823) * SNOW-1043081: Adding support for qualified image repository names * SNOW-1043081: Fixing test imports * SNOW-1043081: Adding tests for getting image repository url without db or schema SNOW-1011771: Added generic REPLACE, IF EXISTS, IF NOT EXISTS flags (#826) * SNOW-1011771: Adding generic OR REPLACE, IF EXISTS, IF NOT EXISTS flags to flags.py * SNOW-1011771: Using generic ReplaceOption in snowpark deploy and streamlit deploy * SNOW-1011771: Using generic IfNotExistsOption in compute pool create and updating unit tests. * SNOW-1011771: Using generic IfNotExistsOption in service create and updating unit tests * SNOW-1011771: Using generic ReplaceOption and IfNotExistsOption in image-repository create. * SNOW-1011771: Fixup * SNOW-1011771: Update release notes * SNOW-1011771: Update test_help_messages * SNOW-1011771: precommit * SNOW-1011771: Adding validation that only one create mode option can be set at once * fixup * SNOW-1011771: Updating tests for REPLACE AND IF NOT EXISTS case on image-repository create to throw error * SNOW-1011771: Adding snapshots * SNOW-1011771: Adding a new mutually_exclusive field to OverrideableOption * formatting * SNOW-1011771: Adding tests for OverrideableOption * SNOW-1011771: Fixing test failures due to improperly quoted string Add snow --help to test_help_messages (#821) * Add snow --help to test_help_messages * update snapshot Avoid plain print, make sure silent is eager flag (#871) [NADE] Update CODEOWNERS to use NADE team id. (#873) update to using nade team in codeowners New workflow to stop running workflows after new commit (#862) * new workflow * new workflow * new workflow * new workflow * typo fix * typo fix * import fix * import fix * import fix * import fix * import fix * import fix * import fix * new approach * new approach * new approach * new approach * new approach * New approach * added to test * Added to more workflows * Dummy commit Schemas adjusting native apps to streamlit fixing streamlit fixies after unit tests fixies after unit tests fixing for snowflake fixing for snowflake Fixes after review Fixes after review Fixes after review * Fixes after review * Implemented error class * Fixes * Fixes * Fixes * Fixes * typo fix * Added unit test * Added unit test * Fixes after review * Fixes after review * Fixes * Fixes * Fixes --------- Co-authored-by: Adam Stus <[email protected]>

sfc-gh-jsikorski commented Mar 5, 2024

View reviewed changes