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.

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

Moved Snowpark and Streamlit artifacts to separate directory in outpu… #1909

Conversation

sfc-gh-astus
Copy link
Contributor

@sfc-gh-astus sfc-gh-astus commented Dec 2, 2024

…t/bundle

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

Moved Snowpark and Streamlit artifacts to separate directory in output/bundle

@sfc-gh-astus sfc-gh-astus requested a review from a team as a code owner December 2, 2024 14:21
@sfc-gh-astus sfc-gh-astus force-pushed the moved-snowpark-and-streamlit-to-separate-directories branch 2 times, most recently from 6b61c7f to 20a232b Compare December 2, 2024 15:08
@sfc-gh-astus sfc-gh-astus requested a review from a team as a code owner December 2, 2024 15:08
@sfc-gh-astus sfc-gh-astus force-pushed the moved-snowpark-and-streamlit-to-separate-directories branch 3 times, most recently from 7fd63d2 to 7735878 Compare December 2, 2024 16:02
Comment on lines 81 to 84
@property
def deploy_root(self) -> Path:
return self.project_root / "output" / "deploy" / "snowpark"

Copy link
Contributor

Choose a reason for hiding this comment

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

This path is build in multiple places in repeated manner.
How about creating a common tool for a deploy root path

from pathlib import Path


def deploy_root(root: Path, app_type: str | None = None) -> Path:
    if app_type:
        return root / "output" / "deploy" / app_type
    return root / "output" / "deploy"

@property
def deploy_root(self) -> Path:
   return deploy_root(self.project_root, "snowpark")

@property
def deploy_root(self) -> Path:
   return deploy_root(self.project_root, "streamlit")

@property
def deploy_root(self) -> Path:
   return deploy_root(self.project_root)

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

@sfc-gh-astus sfc-gh-astus force-pushed the moved-snowpark-and-streamlit-to-separate-directories branch from 7735878 to 877d282 Compare December 3, 2024 11:43
Comment on lines 19 to 22
def deploy_root(root: Path, app_type: str | None = None) -> Path:
if app_type:
return root / "output" / "deploy" / app_type
return root / "output" / "deploy"
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Native apps have made this a configurable property in snowflake.yml since day 1. I think all domains should support that as well.
  2. This isn't resilient to having multiple entities of the same type, which is a big deal if we start deploying multiple entities in a dependency chain. I think we should either use output/deploy, as native apps do, for all domains now (it's consistent and simple for now), or make output/deploy/<entity id> work across the board. That last one is probably significantly bigger in terms of implementation however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I will add.
  2. You mean to mix artifacts from all entities in output/deploy? I added output/deploy/snowpark and output/deploy/streamlit to separate artifacts for each entity type, to keep similar solution what we have right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

there won't be any mixing, each bundle/deploy step should wipe out output/deploy. Segregating by type doesn't buy us anything since multiple entities for the same type could have conflicting artifacts.

Copy link
Contributor

Choose a reason for hiding this comment

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

About #1, I factored out the model portion on our feature branch already.

@sfc-gh-astus sfc-gh-astus force-pushed the features/unification-artifacts branch from c20fb7c to d181eb7 Compare December 4, 2024 17:03
@sfc-gh-bdufour sfc-gh-bdufour force-pushed the features/unification-artifacts branch from d181eb7 to fe16b12 Compare December 6, 2024 17:53
@sfc-gh-astus sfc-gh-astus force-pushed the moved-snowpark-and-streamlit-to-separate-directories branch 3 times, most recently from 55c6c62 to 785a4a9 Compare December 9, 2024 15:24
@sfc-gh-bdufour sfc-gh-bdufour force-pushed the features/unification-artifacts branch from 85d1c72 to 9cb87b3 Compare December 9, 2024 21:15
@sfc-gh-astus sfc-gh-astus force-pushed the moved-snowpark-and-streamlit-to-separate-directories branch from 785a4a9 to c369f7e Compare December 10, 2024 08:33
@sfc-gh-astus sfc-gh-astus merged commit bf62cfb into features/unification-artifacts Dec 11, 2024
20 checks passed
@sfc-gh-astus sfc-gh-astus deleted the moved-snowpark-and-streamlit-to-separate-directories branch December 11, 2024 10:45
sfc-gh-astus added a commit that referenced this pull request Dec 12, 2024
#1909)

Moved Snowpark and Streamlit artifacts to separate directory in output/deploy
sfc-gh-astus added a commit that referenced this pull request Dec 13, 2024
#1909)

Moved Snowpark and Streamlit artifacts to separate directory in output/deploy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants