Skip to content

Conversation

@raethlein
Copy link
Collaborator

@raethlein raethlein commented Jan 7, 2025

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@raethlein raethlein added security-assessment-completed Security assessment has been completed for PR change:refactor PR contains code refactoring without behavior change impact:users PR changes affect end users labels Jan 7, 2025
@raethlein raethlein changed the title Remove usage of private snowflake-connector APIs [Draft] Remove usage of private snowflake-connector APIs Jan 7, 2025
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems to be the sqlstate which is set by the snowpark library: https://github.com/snowflakedb/snowflake-connector-python/blob/main/src/snowflake/connector/network.py#L584-L596
Since it is an official sql standard, it is better to check for that than for the error ids as we did before.

@raethlein raethlein force-pushed the refactor/no-usage-of-snowpark-private-apis branch from 356f361 to 54debc0 Compare January 8, 2025 10:52
@raethlein raethlein requested a review from vdonato January 8, 2025 10:54
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

LGTM assuming everything works locally / in SiS

@raethlein raethlein changed the title [Draft] Remove usage of private snowflake-connector APIs Remove usage of private snowflake-connector APIs Jan 9, 2025
@raethlein raethlein marked this pull request as ready for review January 9, 2025 12:24
@raethlein raethlein requested a review from a team as a code owner January 9, 2025 12:24
@raethlein
Copy link
Collaborator Author

Just tested it locally and also locally in SiS and everything seems to work as expected!

@raethlein raethlein force-pushed the refactor/no-usage-of-snowpark-private-apis branch from 54debc0 to cae4662 Compare January 9, 2025 12:26
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

Since passing kwargs is not anymore supported in this case, is there anything to update on the docstring/documentation side?

@raethlein raethlein force-pushed the refactor/no-usage-of-snowpark-private-apis branch from cae4662 to f2150bd Compare January 9, 2025 16:48
@raethlein
Copy link
Collaborator Author

raethlein commented Jan 9, 2025

@lukasmasuch that's a good point. In our documentation, we provide an example that seems to contradict with the Snowflake documentation.

The example
Screenshot 2025-01-09 at 17 56 35
indicates that you can use a default connection but set, for example, the username.
But the Snowflake doc states

If you choose to rely on a default connection, you cannot override connection parameters, such as username, database, or schema.

So you cannot leverage the default connection by providing the connection name snowflake but still override fields such as user. I believe we should update that example and state that this is only possible when using in conjunction with streamlit secrets (because this would trigger one if-branch before the modified one).
The Snowflake docs only provide examples where no args are given in order to load the default values, so I think we should adhere to that pattern.

@lukasmasuch
Copy link
Collaborator

Ok, but sounds like this is not supported only if there is a default connection. In other cases, you can use kwargs with the snowflake connection. cc @sfc-gh-dmatthews

@raethlein
Copy link
Collaborator Author

yep! then you wouldn't write st.connection("snowflake") but st.connection("my_connection_name", type="snowflake", **your_kwargs) which would trigger the other code path and pass the your_kwargs.

@sfc-gh-dmatthews
Copy link
Collaborator

sfc-gh-dmatthews commented Jan 9, 2025

Okay, so I just need to add a warning to the examples that if you have configured a default snowflake connection, you must specify a connection name (not "snowflake") to use kwargs or do anything other than use that default connection as-is?

@raethlein
Copy link
Collaborator Author

raethlein commented Jan 9, 2025

If you have a secret [connections.snowflake] then that one is used if you use st.connection("snowflake") and you can pass extra arguments.
Otherwise, if you don't have that secret and have configured a default connection and want to use it, you just provide st.connection("snowflake") with no extra arguments.
Or use st.connection("my_connection", type="snowflake") with additional kwargs.

@raethlein raethlein force-pushed the refactor/no-usage-of-snowpark-private-apis branch from c825b3f to c82b065 Compare January 10, 2025 10:01
@raethlein raethlein force-pushed the refactor/no-usage-of-snowpark-private-apis branch from c82b065 to 6228ac5 Compare January 10, 2025 16:01
>>> df = conn.query("SELECT * FROM my_table")
**Example 6: Default connection in Snowflake's connection configuration file**
***Example 6: Default connection in Snowflake's connection configuration file***
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sfc-gh-dmatthews @lukasmasuch I have updated the docstring to capture whats going on. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to reformat it a bit. Would you prefer a commit or PR to your branch with updates? Or do you want me to do a fast follow? @raethlein

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sfc-gh-dmatthews Whatever works best for you 🙂 both is fine for me!

@raethlein raethlein merged commit 9a67af5 into develop Jan 14, 2025
36 checks passed
edegp pushed a commit to edegp/streamlit that referenced this pull request Jan 19, 2025
## Describe your changes

- Bump min version of snowflake-connector to where the
connection-manager definitely existed
- Remove usage of private APIs and leverage the default connection
handling as documented in
https://docs.snowflake.cn/en/developer-guide/python-connector/python-connector-connect#setting-a-default-connection
- When using the `"snowflake"` connection, no `kwargs` are passed to
`snowflake.connector.connect` anymore to follow the [official Snowflake
documentation](https://docs.snowflake.cn/en/developer-guide/python-connector/python-connector-connect#setting-a-default-connection)

## GitHub Issue Link (if applicable)

## Testing Plan

- Explanation of why no additional tests are needed
- Unit Tests (JS and/or Python)
- E2E Tests
- Any manual testing needed?

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
@lukasmasuch lukasmasuch deleted the refactor/no-usage-of-snowpark-private-apis branch March 13, 2025 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:refactor PR contains code refactoring without behavior change impact:users PR changes affect end users security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants