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

Added snow spcs compute-pool deploy command #2001

Merged
merged 2 commits into from
Jan 20, 2025
Merged

Added snow spcs compute-pool deploy command #2001

merged 2 commits into from
Jan 20, 2025

Conversation

sfc-gh-astus
Copy link
Contributor

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

Added new command snow spcs compute-pool deploy using snowflake.yml.
Added --replace flag for snow spcs compute-pool create command.

@sfc-gh-astus sfc-gh-astus requested review from a team as code owners January 17, 2025 11:46
@sfc-gh-jsikorski sfc-gh-jsikorski self-requested a review January 17, 2025 12:41
sfc-gh-jsikorski
sfc-gh-jsikorski reviewed Jan 17, 2025
View reviewed changes
src/snowflake/cli/_plugins/spcs/compute_pool/compute_pool_entity_model.py Outdated

class ComputePoolEntityModel(EntityModelBase):
type: Literal["compute-pool"] = DiscriminatorField() # noqa: A003
min_nodes: Optional[int] = Field(title="Minimum number of nodes", default=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

This number should be greater than 0. Please add validator.

Also - documentation says it's a required parameter https://docs.snowflake.com/en/sql-reference/sql/create-compute-pool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

src/snowflake/cli/_plugins/spcs/compute_pool/compute_pool_entity_model.py
auto_suspend_seconds: Optional[int] = Field(
title="Number of seconds of inactivity after which you want Snowflake to automatically suspend the compute pool",
default=3600,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the command, there is also a comment field - is there a reason ,that it was ommited here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

src/snowflake/cli/_plugins/spcs/compute_pool/commands.py
@@ -124,6 +136,7 @@ def create(
),
auto_suspend_secs: int = AutoSuspendSecsOption(),
comment: Optional[str] = CommentOption(help=_COMMENT_HELP),
replace: bool = ReplaceOption(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be set to False by default, to ensure backwards compatibility?

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 is

ReplaceOption = OverrideableOption(
    False,
    "--replace",

src/snowflake/cli/_plugins/spcs/compute_pool/commands.py
project_type="compute-pool", project_root=cli_context.project_root
)

if entity_id and entity_id not in compute_pools:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if entity_id and entity_id not in compute_pools:
if entity_id not in compute_pools:

entity_id is a required parameter - if an empty string is passed current check wouldn't work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be required? I copied logic from Streamlit, you can not provide entity_id and it will work if in snowflake.yml is only one compute pool.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, didn't notice that it defaults to None

src/snowflake/cli/_plugins/spcs/compute_pool/commands.py Outdated
if entity_id and entity_id not in compute_pools:
raise UsageError(f"No '{entity_id}' entity in project definition file.")

if len(compute_pools.keys()) == 1:
Copy link
Contributor

Choose a reason for hiding this comment

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

If we know that entity_id is a key in compute_pools this check seems unnecesarry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to elif

src/snowflake/cli/_plugins/spcs/compute_pool/manager.py
@@ -39,7 +44,17 @@ def create(
auto_suspend_secs: int,
comment: Optional[str],
if_not_exists: bool,
replace: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this should be an optional parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ye, but flag is not optional, has default.

src/snowflake/cli/_plugins/spcs/compute_pool/manager.py Outdated

if replace:
object_manager = ObjectManager()
object_type = str(ObjectType.COMPUTE_POOL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
object_type = str(ObjectType.COMPUTE_POOL)
object_type = ObjectType.COMPUTE_POOL.sf_name

It's the same, but seems more informative

@sfc-gh-astus sfc-gh-astus force-pushed the compute-pool-model-in-snowflake-yml branch 2 times, most recently from 88767b8 to f9964e4 Compare January 20, 2025 08:41
@sfc-gh-astus sfc-gh-astus force-pushed the compute-pool-model-in-snowflake-yml branch from f9964e4 to 9d2b6eb Compare January 20, 2025 11:56
@sfc-gh-astus sfc-gh-astus merged commit cd0ff29 into main Jan 20, 2025
18 of 20 checks passed
@sfc-gh-astus sfc-gh-astus deleted the compute-pool-model-in-snowflake-yml branch January 20, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-jsikorski sfc-gh-jsikorski sfc-gh-jsikorski left review comments

@sfc-gh-mraba sfc-gh-mraba sfc-gh-mraba approved these changes

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-astus @sfc-gh-jsikorski @sfc-gh-mraba