-
Notifications
You must be signed in to change notification settings - Fork 11
fix(core): write use_polars_sort to Ray DatasetContext #355
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
Changes from 3 commits
d848869
1597a5e
27bd413
5dc380d
9352f37
a735002
746c3a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,8 @@ | |
| from typing import Any, Literal | ||
|
|
||
| import dagster as dg | ||
| from pydantic import Field | ||
| from packaging.version import Version | ||
| from pydantic import Field, model_validator | ||
|
|
||
| USER_DEFINED_RAY_KEY = "dagster-ray/config" | ||
| DAGSTER_RAY_NAMESPACES_ENV_VAR = "DAGSTER_RAY_NAMESPACES" | ||
|
|
@@ -77,7 +78,32 @@ class RayDataExecutionOptions(dg.Config): | |
| cpu_limit: int = 5000 | ||
| gpu_limit: int = 0 | ||
| verbose_progress: bool = True | ||
| use_polars: bool = True | ||
| use_polars_sort: bool = Field( | ||
| default=True, | ||
| description="Whether to use Polars for sort operations in Ray Data. Forwarded to `ray.data.DatasetContext.use_polars_sort`.", | ||
| ) | ||
| use_polars: bool | None = Field( | ||
| default=None, | ||
| description="Deprecated. Use `use_polars_sort` instead. Ray 2.55 renamed the underlying `DatasetContext` attribute to `use_polars_sort`.", | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to change the description, we have the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 5dc380d — removed the custom |
||
| deprecated="`use_polars` is deprecated; use `use_polars_sort` instead.", | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add "is deprecated and will be removed in dagster-ray 0.5.0"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 5dc380d: ``use_polars |
||
| ) | ||
|
|
||
| @model_validator(mode="before") | ||
| @classmethod | ||
| def _forward_deprecated_use_polars(cls, data: Any) -> Any: | ||
| if not isinstance(data, dict): | ||
| return data | ||
| legacy = data.get("use_polars") | ||
| new = data.get("use_polars_sort") | ||
| if legacy is not None and new is not None and legacy != new: | ||
| raise ValueError( | ||
| "`use_polars` and `use_polars_sort` were set to contradicting values " | ||
| f"({legacy!r} vs {new!r}). `use_polars` is deprecated; set only " | ||
| "`use_polars_sort`." | ||
| ) | ||
| if legacy is not None and new is None: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is dead code because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored in a735002 to use explicit |
||
| data = {**data, "use_polars_sort": legacy} | ||
| return data | ||
|
|
||
| def apply(self): | ||
| import ray | ||
|
|
@@ -92,7 +118,14 @@ def apply(self): | |
| ) | ||
|
|
||
| ctx.verbose_progress = self.verbose_progress | ||
| ctx.use_polars = self.use_polars | ||
| # Ray >=2.50 renamed the sort-time Polars toggle from `use_polars` to | ||
| # `use_polars_sort`; read the effective value (the legacy field wins when | ||
| # both are set only because the validator has already unified them). | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain this comment? I am seeing the opposite in the validator: in general the new parameter should be preferred - even tho we did explicitly forbid conflicts
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — the comment was wrong. Fixed in 5dc380d: dropped the confusing comment |
||
| effective = self.use_polars if self.use_polars is not None else self.use_polars_sort | ||
| if Version(ray.__version__) >= Version("2.50"): | ||
| ctx.use_polars_sort = effective | ||
| else: | ||
| ctx.use_polars = effective | ||
|
|
||
| def apply_remote(self): | ||
| import ray | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import warnings | ||
|
|
||
| import pytest | ||
| from pydantic import ValidationError | ||
|
|
||
| from dagster_ray.configs import RayDataExecutionOptions | ||
|
|
||
|
|
||
| def test_use_polars_sort_defaults_to_true(): | ||
| opts = RayDataExecutionOptions() | ||
| assert opts.use_polars_sort is True | ||
| # accessing the deprecated field emits a warning, so silence it for this assertion | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", DeprecationWarning) | ||
| assert opts.use_polars is None | ||
|
|
||
|
|
||
| def test_deprecated_use_polars_is_forwarded_to_use_polars_sort(): | ||
| opts = RayDataExecutionOptions(use_polars=False) | ||
| assert opts.use_polars_sort is False | ||
| # reading the deprecated attribute warns and preserves backward compatibility | ||
| with pytest.warns(DeprecationWarning, match="use_polars_sort"): | ||
| assert opts.use_polars is False | ||
|
|
||
|
|
||
| def test_use_polars_and_use_polars_sort_in_agreement_is_accepted(): | ||
| opts = RayDataExecutionOptions(use_polars=True, use_polars_sort=True) | ||
| assert opts.use_polars_sort is True | ||
|
|
||
|
|
||
| def test_use_polars_and_use_polars_sort_contradicting_raises(): | ||
| with pytest.raises(ValidationError, match="contradicting values"): | ||
| RayDataExecutionOptions(use_polars=True, use_polars_sort=False) | ||
| with pytest.raises(ValidationError, match="contradicting values"): | ||
| RayDataExecutionOptions(use_polars=False, use_polars_sort=True) | ||
|
|
||
|
|
||
| def test_use_polars_sort_alone_does_not_warn_on_construction(): | ||
| with warnings.catch_warnings(record=True) as caught: | ||
| warnings.simplefilter("always") | ||
| opts = RayDataExecutionOptions(use_polars_sort=False) | ||
| ours = [w for w in caught if "use_polars" in str(w.message)] | ||
| assert ours == [] | ||
| assert opts.use_polars_sort is False |
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.
do not edit the changelog, it's auto-generated by
git-clifffrom conventional commits history.You can add a
## Changelogsection to the PR description for a more verbose changeling entry.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.
Reverted in 5dc380d — the
## Unreleasedblock is removed. I'll add a## Changelogsection to the PR description instead.