Improve config handling in cpp and python #644

kingcrimsontianyu · 2025-02-24T19:12:12Z

Closes #642
Closes #526

This PR makes the following changes:

Improve config setter/getter

This PR lets the properties in kvikio.defaults and kvikio.cufile_driver be set using a friendly Pythonic interface (C++ interface has also been adjusted for improved clarity). The expression below can appear standalone which sets the properties globally, or in a with statement which sets the properties via a context manager.

To set one or more properties

# Set the property globally.
kvikio.defaults.set({"prop1": value1, "prop2": value2})
kvikio.cufile_driver.set({"prop1": value1, "prop2": value2})

# Set the property with a context manager.
# The property automatically reverts to its old value
# after leaving the `with` block.
with kvikio.defaults.set({"prop1": value1, "prop2": value2}):
with kvikio.cufile_driver.set({"prop1": value1, "prop2": value2}):
    ...

To set a single property

# Set the property globally.
kvikio.defaults.set("prop", value)
kvikio.cufile_driver.set("prop", value)

# Set the property with a context manager.
# The property automatically reverts to its old value
# after leaving the `with` block.
with kvikio.defaults.set("prop", value):
with kvikio.cufile_driver.set("prop", value):
    ...

In addition, this PR supports the getter method:

p = kvikio.defaults.get("prop",)
p = kvikio.cufile_driver.get("prop",)

Deprecate the old ways of setting configs

The <prop>_reset and set_<prop> functions are now deprecated. Deprecation warnings (of type FutureWarning) are issued at runtime.

Add Python function is_compat_mode_preferred

The C++ function kvikio::defaults::is_compat_mode_preferred() resolves and queries the current global compat mode. This PR introduces this function to Python.

Python documentation update

copy-pr-bot · 2025-02-24T19:12:16Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

kingcrimsontianyu · 2025-02-24T19:33:38Z

/ok to test

kingcrimsontianyu · 2025-02-24T20:06:02Z

/ok to test

kingcrimsontianyu · 2025-02-24T22:59:37Z

Forgot about cuFile's DriverProperties. Sending this PR to draft and working on that.

kingcrimsontianyu · 2025-02-25T04:56:33Z

/ok to test

cpp/include/kvikio/defaults.hpp

python/kvikio/kvikio/defaults.py

kingcrimsontianyu · 2025-02-26T01:23:30Z

/ok to test

kingcrimsontianyu · 2025-02-26T16:02:35Z

/ok to test

kingcrimsontianyu · 2025-02-26T16:07:33Z

/ok to test

kingcrimsontianyu · 2025-02-26T16:16:49Z

python/kvikio/kvikio/cufile_driver.py

-# TODO: Wrap nicely, maybe as a dataclass?
-# <https://github.com/rapidsai/kvikio/issues/526>
-DriverProperties = cufile_driver.DriverProperties
+properties = cufile_driver.DriverProperties()


I think having an instance of DriverProperties() would suffice. So now the settable properties in cufile driver can be set in the following ways:

kvikio.cuda_driver.properties.<prop> = <value> kvikio.cuda_driver.set("<prop>", <value>) # Can also be used in a "with" statement kvikio.cuda_driver.set({"<prop>": <value>}) # Can also be used in a "with" statement

Let me know if this looks good enough to you @madsbk

madsbk

Don't we also want to use get for configs like task_size ?

python/kvikio/kvikio/cufile_driver.py

madsbk · 2025-02-27T07:30:01Z

python/kvikio/kvikio/cufile_driver.py

+    - To set one or more properties
+
+      .. code-block:: python
+
+         kvikio.cufile_driver.properties.set({"prop1": value1, "prop2": value2})
+
+    - To set a single property
+
+      .. code-block:: python
+
+         kvikio.cufile_driver.properties.set("prop", value)


Include a context example?

kingcrimsontianyu · 2025-02-28T21:37:22Z

python/kvikio/kvikio/_lib/defaults.pyx

@@ -14,80 +14,86 @@ cdef extern from "<kvikio/defaults.hpp>" namespace "kvikio" nogil:
        OFF = 0
        ON = 1
        AUTO = 2
+    bool cpp_is_compat_mode_preferred \


This additional C++ function is needed.

kingcrimsontianyu · 2025-02-28T21:41:30Z

python/kvikio/tests/test_basic_io.py

@@ -100,17 +98,16 @@ def test_incorrect_open_mode_error(tmp_path, xp):


 @pytest.mark.skipif(
-    kvikio.defaults.compat_mode(),


This is a minor bug. By default, compat mode is AUTO, not ON. Here we need to resolve it by using is_compat_mode_preferred() (which necessitates importing its C++ function as mentioned above).

kingcrimsontianyu · 2025-02-28T21:42:33Z

docs/source/api.rst

@@ -11,6 +11,24 @@ CuFile
 .. autoclass:: IOFuture
    :members:

+CuFile driver


Added this section, which is missing on the dev branch.

kingcrimsontianyu · 2025-02-28T21:46:29Z

Don't we also want to use get for configs like task_size ?

Done. Added the get method for both kvikio.defaults and kvikio.cufile_driver.

madsbk

Looks great!
Only question is if we should raise a DeprecationWarning ?

TomAugspurger · 2025-03-03T12:38:51Z

Only question is if we should raise a DeprecationWarning ?

My preference is usually for a FutureWarning if we want people to actually see them (outside of pytest invoked unit tests, which un-hides DeprecationWarnings).

An exception to this is if it's a component that's typically used by other libraries. Maybe we do want a DeprecationWarning initially so that we can update cuDF to use the new system (I haven't checked if it actually uses the config system anywhere, rather than just docs)? But maybe that's more work than it's worth.

madsbk · 2025-03-03T12:55:21Z

An exception to this is if it's a component that's typically used by other libraries. Maybe we do want a DeprecationWarning initially so that we can update cuDF to use the new system (I haven't checked if it actually uses the config system anywhere, rather than just docs)? But maybe that's more work than it's worth.

Exactly, KvikIO is primarily used by other libraries :)

bdice · 2025-03-03T14:43:56Z

My preference is usually for a FutureWarning if we want people to actually see them (outside of pytest invoked unit tests, which un-hides DeprecationWarnings).

We exclusively use FutureWarning in cuDF and have recommended all RAPIDS libraries should match that. It’s also how most PyData libraries we’ve studied handle deprecations, to ensure visibility.

kingcrimsontianyu · 2025-03-03T15:47:51Z

Thanks for the discussion. Interesting subtle difference between the two. I'll then keep the use of FutureWarning to be consistent with the recommendations.

kingcrimsontianyu · 2025-03-03T16:04:18Z

/merge

kingcrimsontianyu · 2025-03-03T16:05:33Z

Thanks for the review!

## Description KvikIO has changed the function names of the config setters to improve clarity (rapidsai/kvikio#644). This PR updates the setter calls in cuDF accordingly. ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes.

vyasr · 2025-03-04T01:47:43Z

For posterity, some of our historical discussions around DeprecationWarning and FutureWarning can be found in:

Also worth noting that we have a pre-commit hook in cudf to enforce never using DeprecationWarning, see rapidsai/cudf#11251 and rapidsai/cudf#11255.

kingcrimsontianyu added breaking Introduces a breaking change improvement Improves an existing functionality python Affects the Python API of KvikIO c++ Affects the C++ API of KvikIO labels Feb 24, 2025

kingcrimsontianyu self-assigned this Feb 24, 2025

kingcrimsontianyu force-pushed the improve-config branch from b8146e0 to f5652ea Compare February 24, 2025 19:30

kingcrimsontianyu marked this pull request as ready for review February 24, 2025 22:52

kingcrimsontianyu requested review from a team as code owners February 24, 2025 22:52

kingcrimsontianyu requested review from TomAugspurger, madsbk and bdice February 24, 2025 22:53

kingcrimsontianyu marked this pull request as draft February 24, 2025 22:59

madsbk reviewed Feb 25, 2025

View reviewed changes

cpp/include/kvikio/defaults.hpp Outdated Show resolved Hide resolved

madsbk reviewed Feb 25, 2025

View reviewed changes

python/kvikio/kvikio/defaults.py Outdated Show resolved Hide resolved

madsbk mentioned this pull request Feb 25, 2025

Added timeout to kvikio requests #643

Merged

TomAugspurger reviewed Feb 25, 2025

View reviewed changes

python/kvikio/kvikio/defaults.py Outdated Show resolved Hide resolved

python/kvikio/kvikio/defaults.py Outdated Show resolved Hide resolved

python/kvikio/kvikio/defaults.py Show resolved Hide resolved

kingcrimsontianyu marked this pull request as ready for review February 26, 2025 16:11

kingcrimsontianyu commented Feb 26, 2025

View reviewed changes

kingcrimsontianyu added 3 commits February 26, 2025 17:23

Improve config handling in cpp and python

593b9e0

Solve rebase conflict

9bffcd7

Minor fixes

de9b461

kingcrimsontianyu added 9 commits February 26, 2025 17:23

Update doc

113179a

Add more tests to defaults

03729be

Add more comments

f2c110d

Add more comments

d1b5f1b

Address review comments

482a58a

Minor adjustment

c0e4357

Add back deprecated python api

dce87db

Add context manager for cufile driver properties

d1bd127

Small fixes

33b133e

kingcrimsontianyu force-pushed the improve-config branch from 55e849c to 33b133e Compare February 26, 2025 22:23

madsbk reviewed Feb 28, 2025

View reviewed changes

kingcrimsontianyu added 3 commits February 28, 2025 12:02

Address review comments

910ed6b

Update docs

b612086

Minor fixes

4a6570a

kingcrimsontianyu commented Feb 28, 2025

View reviewed changes

Fix doc

3eb85f9

madsbk approved these changes Mar 3, 2025

View reviewed changes

kingcrimsontianyu mentioned this pull request Mar 3, 2025

Update calls to KvikIO's config setter rapidsai/cudf#18144

Merged

3 tasks

rapids-bot bot merged commit 4bd58d9 into rapidsai:branch-25.04 Mar 3, 2025
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve config handling in cpp and python #644

Improve config handling in cpp and python #644

kingcrimsontianyu commented Feb 24, 2025 •

edited

Loading

copy-pr-bot bot commented Feb 24, 2025

kingcrimsontianyu commented Feb 24, 2025

kingcrimsontianyu commented Feb 24, 2025

kingcrimsontianyu commented Feb 24, 2025

kingcrimsontianyu commented Feb 25, 2025

kingcrimsontianyu commented Feb 26, 2025

kingcrimsontianyu commented Feb 26, 2025

kingcrimsontianyu commented Feb 26, 2025

kingcrimsontianyu Feb 26, 2025

madsbk left a comment

madsbk Feb 27, 2025

kingcrimsontianyu Feb 28, 2025

kingcrimsontianyu Feb 28, 2025

kingcrimsontianyu Feb 28, 2025

kingcrimsontianyu Feb 28, 2025

kingcrimsontianyu commented Feb 28, 2025

madsbk left a comment

TomAugspurger commented Mar 3, 2025 •

edited

Loading

madsbk commented Mar 3, 2025

bdice commented Mar 3, 2025

kingcrimsontianyu commented Mar 3, 2025

kingcrimsontianyu commented Mar 3, 2025

kingcrimsontianyu commented Mar 3, 2025

vyasr commented Mar 4, 2025

		@@ -100,17 +98,16 @@ def test_incorrect_open_mode_error(tmp_path, xp):


		@pytest.mark.skipif(
		kvikio.defaults.compat_mode(),

Improve config handling in cpp and python #644

Improve config handling in cpp and python #644

Conversation

kingcrimsontianyu commented Feb 24, 2025 • edited Loading

Improve config setter/getter

Deprecate the old ways of setting configs

Add Python function is_compat_mode_preferred

Python documentation update

copy-pr-bot bot commented Feb 24, 2025

kingcrimsontianyu commented Feb 24, 2025

kingcrimsontianyu commented Feb 24, 2025

kingcrimsontianyu commented Feb 24, 2025

kingcrimsontianyu commented Feb 25, 2025

kingcrimsontianyu commented Feb 26, 2025

kingcrimsontianyu commented Feb 26, 2025

kingcrimsontianyu commented Feb 26, 2025

kingcrimsontianyu Feb 26, 2025

Choose a reason for hiding this comment

madsbk left a comment

Choose a reason for hiding this comment

madsbk Feb 27, 2025

Choose a reason for hiding this comment

kingcrimsontianyu Feb 28, 2025

Choose a reason for hiding this comment

kingcrimsontianyu Feb 28, 2025

Choose a reason for hiding this comment

kingcrimsontianyu Feb 28, 2025

Choose a reason for hiding this comment

kingcrimsontianyu Feb 28, 2025

Choose a reason for hiding this comment

kingcrimsontianyu commented Feb 28, 2025

madsbk left a comment

Choose a reason for hiding this comment

TomAugspurger commented Mar 3, 2025 • edited Loading

madsbk commented Mar 3, 2025

bdice commented Mar 3, 2025

kingcrimsontianyu commented Mar 3, 2025

kingcrimsontianyu commented Mar 3, 2025

kingcrimsontianyu commented Mar 3, 2025

vyasr commented Mar 4, 2025

kingcrimsontianyu commented Feb 24, 2025 •

edited

Loading

TomAugspurger commented Mar 3, 2025 •

edited

Loading