-
Notifications
You must be signed in to change notification settings - Fork 79
SNOW-2665953: Make Streamlit Deployment with Versioned Stages as Default in Snow CLI #2685
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
base: main
Are you sure you want to change the base?
Conversation
18ef70d to
97f75ad
Compare
sfc-gh-jwilkowski
left a comment
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.
Thanks for your contribution! For the most part it looks really good, I'd only like to ask to pay attention to repetitive comments bloat and making user-facing messages clear to them. I don't think that all of that terminology is a common knowledge so it would be better to explain those in a different way. Please also take a look at failing tests
sfc-gh-jwilkowski
left a comment
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.
A couple of comments:
- Please add an entry in RELEASE-NOTES.md in
Unreleased version sectiondescribing briefly this change - please edit the description of PR and remove the part about a breaking change - I don't think it's a BCR since we're changing internals of created streamlit, so I don't want to confuse people reading this PR
- There's one case that I'm not sure about - it's possible to deploy a streamlit with
--replaceflag. If that app existed before, there's a chance that we'd replace old-style streamlit app with new style one. I'm not sure if there would be any negative consequences of that, but feels like something we should handle.
Addressed |
## Changes ### CLI Behavior - `snow streamlit deploy` now uses versioned deployment (FROM syntax) by default - Added `--legacy` flag to use old ROOT_LOCATION deployment - `--experimental` flag is now deprecated but still works (shows warning) ### Code Changes - Added `--legacy` flag to streamlit deploy command - Inverted deployment logic: `if experimental` → `if not legacy` - Renamed `_deploy_experimental()` → `_deploy_versioned()` - Removed `ENABLE_STREAMLIT_VERSIONED_STAGE` feature flag - Updated `_is_spcs_runtime_v2_mode()` to not require experimental flag - SPCS runtime v2 (compute pools) only available in versioned mode ### Test Updates - Added `--legacy` flag to 12 unit tests expecting ROOT_LOCATION behavior - Removed `--experimental` flag from tests now using default behavior - Removed `with_feature_flags` wrapper and parametrization - Updated integration tests to reflect new default ## Breaking Change The default deployment mode changes from ROOT_LOCATION to versioned deployment. Users who want the old behavior must use `--legacy` flag.
For consistency with _deploy_versioned(), extracted the else block's deployment logic into a dedicated _deploy_legacy() method. This improves code organization and makes the deploy() method cleaner.
Changed from 'if not legacy' to 'if legacy' for better readability. The positive condition directly maps to the flag name and avoids mental negation.
- Split long comment in _deploy_legacy() method - Break deprecation warning string across multiple lines - Format long test invocation call - Fix typo: elswhere → elsewhere
Update test_streamlit_flow and test_streamlit_deploy_prune_flag to work with
versioned Streamlit deployment (now the default behavior).
Changes:
- test_streamlit_flow: Update stage paths from legacy app_1_stage to versioned
snow://streamlit/{DB}.{SCHEMA}.app_1/versions/live/
- test_streamlit_deploy_prune_flag: Rewrite to use managed versioned stages
instead of user-created stages
Files are now uploaded to managed stages at versions/live/ path instead of
user-specified ROOT_LOCATION stages.
- Add mock for describe() method to return versioned stage path - Update test_deploy to use legacy=True for testing old behavior - Replace experimental parameter with legacy parameter in SPCS runtime v2 tests - Fix test_deploy_with_spcs_runtime_v2_and_legacy_flag_raises_error invalid parameters Changes reflect that versioned deployment is now the default, and the experimental flag has been replaced with a legacy flag to opt into old behavior.
- Add --legacy flag to streamlit deploy help text - Remove oauth-token-request-url parameter (deprecated) These changes reflect the new default versioned deployment behavior.
…commands except streamlit The previous commit incorrectly removed oauth-token-request-url from ALL commands. This parameter should only be excluded from streamlit.deploy command, but remain in all other commands' help text.
This parameter is a standard connection option that applies to all commands including streamlit. Cannot be removed from just streamlit.deploy.
…lit stage Snowflake requires overwrite=true when uploading to streamlit versioned stages. The test was failing because stage copy needs --overwrite flag when uploading to snow://streamlit/ managed stages.
Versioned stages are managed by Snowflake's FBE and have different file lifecycle behavior. The prune flag functionality is primarily for legacy ROOT_LOCATION stages where the CLI has full control over file management. Changed the test to use --legacy flag to properly test the prune functionality on legacy stages instead of versioned stages.
…uce comment bloat
…s to top, add deployment style change warnings
f4cfb7f to
731b0da
Compare
…ent database context
…t deploy commands
…test_database fixture env var
1f5027a to
81a480e
Compare
81a480e to
78d7301
Compare
SNOW-2665953: Make Streamlit Deployment with Versioned Stages as Default in Snow CLI
Summary
Changes the default Streamlit deployment mode from ROOT_LOCATION to versioned stages
Motivation
Changes
CLI Behavior
Implementation
--legacyflag for ROOT_LOCATION deploymentif legacyvsif not legacy)_deploy_experimental()→_deploy_versioned()+ extracted_deploy_legacy()ENABLE_STREAMLIT_VERSIONED_STAGEfeature flagTest Updates
--legacy--experimentalfrom tests expecting default behaviorDeployment Behavior Change
Default mode changes: The
snow streamlit deploycommand now uses versioned stages by default instead of ROOT_LOCATION.Impact
Using ROOT_LOCATION (Legacy Mode)
Users who need ROOT_LOCATION deployment can use the
--legacyflag:Or in CI/CD:
Backward Compatibility
--experimentalflag still works (shows deprecation warning)Testing
Pre-review checklist
...