SNOW-1418523 Make Session thread safe #2312

sfc-gh-aalam · 2024-09-17T21:31:06Z

Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Fixes SNOW-1418523
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
  - If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
- I am adding new logging messages
- I am adding a new telemetry message
- I am adding new credentials
- I am adding a new dependency
- If this is a new feature/behavior, I'm adding the Local Testing parity changes.
- I acknowledge that I have ensured my changes to be thread-safe.
Please describe how your code solves the related issue.

This PR is merging back add updates from feature branch to main branch. It includes the following changes:
1. Core session variables to enable concurrent df operations and queries: SNOW-1418523 make analyzer server connection thread safe #2282
2. Concurrent Files I/O: SNOW-1418523: concurrent file operations #2288
3. UDxF/Sproc updates: SNOW-1418523: make udf and sproc registration thread safe #2289
4. Session parameters and configs: SNOW-1663726 make session config updates thread safe #2302
5. Temp table cleaner updates: SNOW-1663726 make temp table cleaner thread safe #2309
6. Telemetry collection: SNOW-1642189: collect telemetry about concurrency usage #2316
7. Add merge-gate: SNOW-1546090 add merge gate for future thread safe updates #2323

…iables-thread-safe

github-actions · 2024-09-25T16:50:21Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-09-25T17:00:47Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-09-25T17:07:17Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-02T21:55:33Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

…iables-thread-safe

github-actions · 2024-10-03T00:40:22Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

src/snowflake/snowpark/session.py

github-actions · 2024-10-04T16:59:03Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-04T17:07:37Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-04T17:22:15Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-04T17:39:34Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-04T20:11:27Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-04T22:04:22Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

sfc-gh-jrose

For other reviewers: Turning off whitespace comparison simplifies quite a bit.

.github/pull_request_template.md

src/snowflake/snowpark/_internal/compiler/plan_compiler.py

github-actions · 2024-10-05T00:42:07Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

…iables-thread-safe

github-actions · 2024-10-08T00:25:12Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

sfc-gh-yixie · 2024-10-11T16:09:06Z

CHANGELOG.md

@@ -6,6 +6,8 @@

 #### New Features

+- Updated `Session` class to be thread-safe. This allows concurrent query submission, dataframe operations, UDF and store procedure registration, and concurret file uploads.


Typo: concurret

sfc-gh-yixie · 2024-10-11T16:09:38Z

CHANGELOG.md

@@ -6,6 +6,8 @@

 #### New Features

+- Updated `Session` class to be thread-safe. This allows concurrent query submission, dataframe operations, UDF and store procedure registration, and concurret file uploads.
+  - One limitation is that updating `Session` configurations inside multiple-threads may cause other active thread to break. Please update `Session` configurations before starting multiple threads.


This details isn't needed in the changelog. It should be somewhere else in the doc.

sfc-gh-yixie · 2024-10-11T16:18:10Z

src/snowflake/snowpark/_internal/server_connection.py

@@ -184,6 +185,15 @@ def __init__(
            "_skip_upload_on_content_match" in signature.parameters
        )

+    @property
+    def _cursor(self) -> SnowflakeCursor:
+        if not hasattr(self._thread_store, "cursor"):


is hasattr() itself thread-safe?

yes, hasattr is read-only operation on thread local object.

sfc-gh-yixie · 2024-10-11T16:26:13Z

src/snowflake/snowpark/_internal/telemetry.py

@@ -38,6 +39,7 @@ class TelemetryField(Enum):
    TYPE_PERFORMANCE_DATA = "snowpark_performance_data"
    TYPE_FUNCTION_USAGE = "snowpark_function_usage"
    TYPE_SESSION_CREATED = "snowpark_session_created"
+    TYPE_CURSOR_CREATED = "snowpark_cursor_created"


Is the purpose to know whether multiple cursor is used?

Yes, this addition is part of collecting telemtry. It will track if a cursor is used in a new thread.

sfc-gh-yixie · 2024-10-11T16:58:12Z

src/snowflake/snowpark/_internal/udf_utils.py

+    runtime_version = (
+        f"{sys.version_info[0]}.{sys.version_info[1]}"
+        if not runtime_version
+        else runtime_version
+    )


Nit: no need to change. Just for your reference:
runtime_version = runtime_version or f"{sys.version_info[0]}.{sys.version_info[1]}"

yep. that's much simpler. I'll update the code.

sfc-gh-yixie · 2024-10-11T17:17:15Z

src/snowflake/snowpark/session.py

-        )
+        with self._package_lock:
+            result_dict = (
+                existing_packages_dict if existing_packages_dict is not None else {}


why the .copy() is removed here?

The original version of this code had this pattern.

result_dict = existing_packages_dict if existing_pacakges_dict else {} ... for package in dependency_packages: name = package.name if name in result_dict: ... else: result["name"] = name

I refactored it the wrong way using this pattern

result_dict = existing_packages_dict.copy() if existing_pacakges_dict else {} ... for package in dependency_packages: name = package.name if name in result_dict: ... else: result["name"] = name if existing_packages_dict: existing_pacakges_dict.update(result_dict)

I noticed this refactor is not correct as the update step can overwrite updates from a different thread.

So in this change, I undid the refactor and updated the code as follows:

with self._package_lock: result_dict = existing_packages_dict if existing_pacakges_dict else {} ... for package in dependency_packages: name = package.name if name in result_dict: ... else: result["name"] = name

sfc-gh-aalam · 2024-10-14T16:50:01Z

SP Precommit run with this change: https://ci-dev-142.int.snowflakecomputing.com/job/PythonStoredProcBuildPrecommitTest/1395/

…iables-thread-safe

github-actions · 2024-10-14T18:05:43Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

github-actions · 2024-10-14T20:55:44Z

Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing

sfc-gh-aalam added 5 commits September 11, 2024 14:04

add locks

fb4ecf6

Merge branch 'main' into aalam-SNOW-1418523-make-internal-session-var…

f720701

…iables-thread-safe

Merge branch 'main' into aalam-SNOW-1418523-make-internal-session-var…

eca13dc

…iables-thread-safe

Merge branch 'main' into aalam-SNOW-1418523-make-internal-session-var…

96949be

…iables-thread-safe

SNOW-1418523 make analyzer server connection thread safe (#2282)

5f140ab

SNOW-1418523: concurrent file operations (#2288)

0624824

SNOW-1418523: make udf and sproc registration thread safe (#2289)

42d6e19

merge with main

801ad6e

Merge branch 'main' into aalam-SNOW-1418523-make-internal-session-var…

c7fa3ae

…iables-thread-safe

sfc-gh-aalam commented Oct 3, 2024

View reviewed changes

src/snowflake/snowpark/session.py Show resolved Hide resolved

SNOW-1663726 make session config updates thread safe (#2302)

5672a1d

SNOW-1663726 make temp table cleaner thread safe (#2309)

bd0528d

SNOW-1642189: collect telemetry about concurrency usage (#2316)

39a07d4

SNOW-1546090 add merge gate for future thread safe updates (#2323)

4d4e257

add plan-builder that was accidentally removed

66374ee

sfc-gh-aalam marked this pull request as ready for review October 4, 2024 21:41

sfc-gh-aalam requested review from a team as code owners October 4, 2024 21:41

sfc-gh-aalam requested review from sfc-gh-vbudati and sfc-gh-dpetersohn October 4, 2024 21:41

sfc-gh-aalam requested review from sfc-gh-jdu, sfc-gh-yuwang and sfc-gh-jrose October 4, 2024 21:41

changelog updates

8e57d95

sfc-gh-jrose reviewed Oct 4, 2024

View reviewed changes

.github/pull_request_template.md Outdated Show resolved Hide resolved

src/snowflake/snowpark/_internal/compiler/plan_compiler.py Show resolved Hide resolved

create hyperlink for doc

822e3f8

Merge branch 'main' into aalam-SNOW-1418523-make-internal-session-var…

860d947

…iables-thread-safe

sfc-gh-jrose approved these changes Oct 9, 2024

View reviewed changes

sfc-gh-yixie reviewed Oct 11, 2024

View reviewed changes

sfc-gh-aalam added 4 commits October 14, 2024 11:01

address comments

805ca91

Merge branch 'main' into aalam-SNOW-1418523-make-internal-session-var…

1583ad9

…iables-thread-safe

fix changelog

9aeb1f1

fix changelog

2e78e4a

sfc-gh-yixie approved these changes Oct 14, 2024

View reviewed changes

address feedback

2d2ff8a

sfc-gh-aalam enabled auto-merge (squash) October 14, 2024 21:01

sfc-gh-aalam merged commit 7c22750 into main Oct 14, 2024
36 checks passed

sfc-gh-aalam deleted the aalam-SNOW-1418523-make-internal-session-variables-thread-safe branch October 14, 2024 21:32

github-actions bot locked and limited conversation to collaborators Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1418523 Make Session thread safe #2312

SNOW-1418523 Make Session thread safe #2312

sfc-gh-aalam commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

sfc-gh-jrose left a comment

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 8, 2024

sfc-gh-yixie Oct 11, 2024

sfc-gh-yixie Oct 11, 2024

sfc-gh-yixie Oct 11, 2024

sfc-gh-aalam Oct 14, 2024

sfc-gh-yixie Oct 11, 2024

sfc-gh-aalam Oct 14, 2024

sfc-gh-yixie Oct 11, 2024

sfc-gh-aalam Oct 14, 2024

sfc-gh-yixie Oct 11, 2024

sfc-gh-aalam Oct 14, 2024

sfc-gh-aalam commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

		@@ -6,6 +6,8 @@

		#### New Features

		- Updated `Session` class to be thread-safe. This allows concurrent query submission, dataframe operations, UDF and store procedure registration, and concurret file uploads.

SNOW-1418523 Make Session thread safe #2312

SNOW-1418523 Make Session thread safe #2312

Conversation

sfc-gh-aalam commented Sep 17, 2024 • edited Loading

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 3, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

sfc-gh-jrose left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-aalam commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

github-actions bot commented Oct 14, 2024

sfc-gh-aalam commented Sep 17, 2024 •

edited

Loading