SNOWVATION: Replace parameter boilerplate with utility classes. #2751

sfc-gh-jrose · 2024-12-11T23:57:48Z

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

Fixes SNOW-NNNNNNN
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. Follow the link for more information: Thread-safe Developer Guidelines
Please describe how your code solves the related issue.

The goal of this refactor is to provide a central location for defining parameters and other snowpark-python configuration. This should make it easier to add additional parameters in the future as well as gate more deprecated or exerimental logic behind parameters.

Session parmeters are now all added in Session._initialize_config. All definition needed for them should happen there.
Package level confuguration settings should similarly all be defined in src/snowflake/snowpark/_internal/config.py

Parameters and package level configs can all be accessed via session.conf.

As part of this change I've moved all of the session level parameters over to the new settings style, but there are likely package level configuration options that I have missed. For now the parameters that are exposed as properties of the session object remain available in this manner, but in the future we should standardize the access method to be through the config object.

sfc-gh-aalam · 2024-12-14T00:08:39Z

src/snowflake/snowpark/session.py

-    def conf(self) -> RuntimeConfig:
+    def conf(self) -> SettingStore:


we should probably keep the name same for migration sake: https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.conf.RuntimeConfig.html

sfc-gh-aalam · 2024-12-14T00:09:50Z

tests/unit/test_session.py

 @pytest.mark.parametrize(
-    "parameter_name",
-    [
-        "_auto_clean_up_temp_table_enabled",
-        "_cte_optimization_enabled",
-        "_large_query_breakdown_enabled",
-    ],
+    "parameter_name", ["auto_clean_up_temp_table_enabled", "cte_optimization_enabled"]
 )


merge artifact

sfc-gh-aalam · 2024-12-14T00:11:25Z

tests/integ/test_temp_table_cleanup.py

+        session.auto_clean_up_temp_table_enabled = True
+    assert session.auto_clean_up_temp_table_enabled is True
    assert "auto_clean_up_temp_table_enabled is experimental" in caplog.text
+    session.auto_clean_up_temp_table_enabled = False


do we not raise warning only when we enable the param?

The warning is only raised when setting to a non-default value.

sfc-gh-aalam · 2024-12-14T00:29:00Z

src/snowflake/snowpark/_internal/config.py

+    """
+
+    name: str
+    description: str | None = field(default=None)


Suggested change

description: str | None = field(default=None)

description: Optional[str] = field(default=None)

nit: we follow this style in the repo.

sfc-gh-aalam · 2024-12-14T00:31:24Z

would it be easy to cover changes made in this PR: #2673

sfc-gh-jrose · 2024-12-14T00:34:06Z

would it be easy to cover changes made in this PR: #2673

That wouldn't be too hard. I think I'd recommend downgrading from an error to a warning though so that users can be aware that they should clean things up.

sfc-gh-aalam

approving based on

re-naming SettingStore to RuntimeConfig so that we don't change names of classes
adding a simple how-to maybe in contributing.md or config.py

sfc-gh-aling · 2025-01-17T07:01:18Z

src/snowflake/snowpark/_internal/config.py

+when all configuration needs to be accessible from a single object.
+"""
+
+import pkg_resources


pkg_resources is kinda deprecated, check notes in https://setuptools.pypa.io/en/latest/pkg_resources.html
is it possible to switch to importlib as suggested?

sfc-gh-aling · 2025-01-17T07:07:20Z

src/snowflake/snowpark/_internal/config.py

+            self._parent is not None
+            and (parent_value := self._parent.value) is not None
+        ):
+            return parent_value


is there a chance of a multi layer setting such that we need recursion here to get the default value?
A.default = 1
B.parent = A , default = None
C.parent = B, default = None

sfc-gh-aling · 2025-01-17T07:09:51Z

src/snowflake/snowpark/_internal/config.py

+        name (str): The name of the setting that is used to reference it.
+        description (str): A docstring that describes the function of the setting.
+        default: (Any): The default value that a setting takes when not otherwise configured.
+        read_only (bool): Disallows modification of the setting when set to True.
+        experimental_since (str): When set this will warn users that changing the value of this
+            setting is experimental and may not be ready for production environments.


it just came to my mind that do we want a field for "deprecation"?
not blocking this PR as we can add in the future

sfc-gh-aling · 2025-01-17T07:10:37Z

src/snowflake/snowpark/_internal/config.py

+        parameter_name (str): The name of the session parameter that holds the value for this setting.
+        synchronize (bool): When set to True the server side session parameter will be updated when
+            this settings value is changed.
+        telemetry_hook (Callable): A callback function that allows emitting telemetry when thissettings


nit

Suggested change

telemetry_hook (Callable): A callback function that allows emitting telemetry when thissettings

telemetry_hook (Callable): A callback function that allows emitting telemetry when this

sfc-gh-aling · 2025-01-17T07:15:30Z

src/snowflake/snowpark/session.py

-                    return getattr(self._session._conn._conn, key)
-                return self._conf.get(key, default)
-
-        def is_mutable(self, key: str) -> bool:


I don't see is_mutable is implemented the new config class?

sfc-gh-aling · 2025-01-17T07:17:27Z

src/snowflake/snowpark/session.py

-    def is_feature_enabled_for_version(self, parameter_name: str) -> bool:
-        """
-        This method checks if a feature is enabled for the current session based on
-        the server side parameter.
-        """
-        version = self._conn._get_client_side_session_parameter(parameter_name, "")
-        return (
-            isinstance(version, str)
-            and version != ""
-            and pkg_resources.parse_version(self.version)
-            >= pkg_resources.parse_version(version)
-        )
-


shall we keep this? removing this is a BCR, also I feel this func can be helpful to users

sfc-gh-jrose added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Dec 11, 2024

sfc-gh-jrose added 6 commits December 12, 2024 11:40

SNOWVATION: Replace parameter boilerplate with utility classes.

cdc440e

Add new file

8e90bef

Test fixes

2b8b578

py3.8 compatibility

b2b4560

clean-up and some test fixes

7cc0204

additional test fixes

0f2d9d2

sfc-gh-jrose force-pushed the jrose_snow_vation_config_refactor branch from 72c910b to 0f2d9d2 Compare December 12, 2024 19:40

sfc-gh-jrose added 2 commits December 12, 2024 11:42

another test fix

54f014f

modin fixes and docstrings

ed33847

github-actions bot added the snowpark-pandas label Dec 12, 2024

sfc-gh-jrose and others added 4 commits December 12, 2024 14:35

another modin fix

941ec27

another modin fix

26585ad

Add unit tests

6bd27f7

Merge branch 'main' into jrose_snow_vation_config_refactor

84375a0

sfc-gh-jrose requested review from sfc-gh-aalam, sfc-gh-aling and sfc-gh-yixie December 14, 2024 00:02

sfc-gh-jrose marked this pull request as ready for review December 14, 2024 00:02

sfc-gh-jrose requested review from a team as code owners December 14, 2024 00:02

sfc-gh-jrose added 2 commits December 13, 2024 16:06

add unit test file

c4a0fda

Add test case for missing update value

93324b0

sfc-gh-aalam reviewed Dec 14, 2024

View reviewed changes

remove future annotations

969e605

sfc-gh-jrose and others added 2 commits December 17, 2024 14:56

Merge branch 'main' into jrose_snow_vation_config_refactor

21a2300

Merge branch 'main' into jrose_snow_vation_config_refactor

113d4d9

sfc-gh-jrose added 7 commits December 18, 2024 14:50

typing

ee6ae11

fix Falsey value bug

d7028e9

fix mock

e51ed12

pyright

2617fcc

pyright

f3b86d8

another test fix

b6f9491

another test fix

417f037

sfc-gh-aalam approved these changes Jan 16, 2025

View reviewed changes

sfc-gh-aling reviewed Jan 17, 2025

View reviewed changes

sfc-gh-yuwang closed this Feb 13, 2025

sfc-gh-yuwang deleted the jrose_snow_vation_config_refactor branch February 13, 2025 22:52

github-actions bot locked and limited conversation to collaborators Feb 13, 2025

sfc-gh-jrose restored the jrose_snow_vation_config_refactor branch February 13, 2025 23:03

sfc-gh-jrose reopened this Feb 13, 2025

		def conf(self) -> RuntimeConfig:
		def conf(self) -> SettingStore:

	description: str \| None = field(default=None)
	description: Optional[str] = field(default=None)

	telemetry_hook (Callable): A callback function that allows emitting telemetry when thissettings
	telemetry_hook (Callable): A callback function that allows emitting telemetry when this

SNOWVATION: Replace parameter boilerplate with utility classes. #2751

Are you sure you want to change the base?

SNOWVATION: Replace parameter boilerplate with utility classes. #2751

Uh oh!

Conversation

sfc-gh-jrose commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-aalam commented Dec 14, 2024

Uh oh!

sfc-gh-jrose commented Dec 14, 2024

Uh oh!

sfc-gh-aalam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sfc-gh-jrose commented Dec 11, 2024 •

edited

Loading