Skip to content

Commit a2e90dc

Browse files
sfc-gh-jkewsfc-gh-mvashishtha
authored andcommitted
FIX-#7684: When we exceed max_cost for all available Backends an error may occur (#7685)
The BackendCostCalculator was originally designed to return None when no suitable backend was available. It would do this when the max_cost of all backend were exceeded. In practice, and with changes related to considering multiple backends for pre-operation switches it was realized that this was too restrictive and it would throw a ValueError under certain workloads. This change allows the calculator to first calculate a backend considering max_cost but, if one is not found, then calculate one disregarding max_cost. A backend should always be returned in this case. ## What do these changes do? - [x] first commit message and PR title follow format outlined [here](https://modin.readthedocs.io/en/latest/development/contributing.html#commit-message-formatting) > **_NOTE:_** If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title. - [x] passes `flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py` - [x] passes `black --check modin/ asv_bench/benchmarks scripts/doc_checker.py` - [x] signed commit with `git commit -s` <!-- you can amend your commit with a signature via `git commit -amend -s` --> - [x] Resolves #7684 - [x] tests added and passing - [x] module layout described at `docs/development/architecture.rst` is up-to-date <!-- if you have added, renamed or removed files or directories please update the documentation accordingly -->
1 parent 69f0e58 commit a2e90dc

File tree

3 files changed

+47
-25
lines changed

3 files changed

+47
-25
lines changed

modin/config/envvars.py

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -508,10 +508,12 @@ def set_active_backends(cls, new_choices: tuple) -> None:
508508
ValueError
509509
Raises a ValueError when the set of new_choices are not already registered
510510
"""
511-
if not all(i in cls._BACKEND_TO_EXECUTION for i in new_choices):
512-
raise ValueError(
513-
f"Active backend choices {new_choices} are not all registered."
514-
)
511+
registered_backends = cls._BACKEND_TO_EXECUTION
512+
for i in new_choices:
513+
if i not in registered_backends:
514+
raise ValueError(
515+
f"Active backend choices {new_choices} are not all registered."
516+
)
515517
cls.choices = new_choices
516518

517519
@classmethod

modin/core/storage_formats/base/query_compiler_calculator.py

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -233,13 +233,31 @@ def calculate(self) -> str:
233233
if cost is not None:
234234
self._add_cost_data(backend_to, cost)
235235

236-
min_value = None
237-
for k, v in self._backend_data.items():
238-
if v.cost > v.max_cost:
239-
continue
240-
if min_value is None or min_value > v.cost:
241-
min_value = v.cost
242-
self._result_backend = k
236+
self._result_backend = None
237+
238+
def get_min_cost_backend(skip_exceeds_max_cost=True) -> str:
239+
result = None
240+
min_value = None
241+
for k, v in self._backend_data.items():
242+
if skip_exceeds_max_cost and v.cost > v.max_cost:
243+
continue
244+
if min_value is None or min_value > v.cost:
245+
min_value = v.cost
246+
result = k
247+
return result
248+
249+
# Get the best backend, skipping backends where we may exceed
250+
# the total cost
251+
self._result_backend = get_min_cost_backend(skip_exceeds_max_cost=True)
252+
253+
# If we still do not have a backend, pick the best backend while
254+
# ignoring max_cost
255+
if self._result_backend is None:
256+
self._result_backend = get_min_cost_backend(skip_exceeds_max_cost=False)
257+
258+
# This should not happen
259+
if self._result_backend is None:
260+
raise ValueError("No backends are available to calculate costs.")
243261

244262
if len(self._backend_data) > 1:
245263
get_logger().info(
@@ -267,10 +285,6 @@ def calculate(self) -> str:
267285
1,
268286
)
269287

270-
if self._result_backend is None:
271-
raise ValueError(
272-
f"Cannot cast to any of the available backends, as the estimated cost is too high. Tried these backends: [{', '.join(self._backend_data.keys())}]"
273-
)
274288
return self._result_backend
275289

276290
def _add_cost_data(self, backend, cost):

modin/tests/pandas/native_df_interoperability/test_compiler_caster.py

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -534,13 +534,6 @@ def test_cast_to_first_backend_with___init__(pico_df, cluster_df):
534534
assert df3.get_backend() == "Cluster" # result should be on cluster
535535

536536

537-
def test_no_solution(pico_df, local_df, cluster_df, cloud_df):
538-
# Backends should appear in the order of arguments, followed by any active backends not present
539-
# among the arguments.
540-
with pytest.raises(ValueError, match=r"Pico, Local_Machine, Cluster, Cloud"):
541-
pd.concat(axis=1, objs=[pico_df, local_df, cluster_df, cloud_df])
542-
543-
544537
def test_self_cost_causes_move(cloud_high_self_df, cluster_df):
545538
"""
546539
Test that ``self_cost`` is being properly considered.
@@ -572,9 +565,9 @@ def test_self_cost_causes_move(cloud_high_self_df, cluster_df):
572565
("cloud_df", "cloud_df", "cloud_df", "cloud_df", "Cloud"),
573566
# moving all dfs to cloud is 1250, moving to cluster is 1000
574567
# regardless of how they are ordered
575-
("pico_df", "local_df", "cluster_df", "cloud_df", None),
576-
("cloud_df", "local_df", "cluster_df", "pico_df", None),
577-
("cloud_df", "cluster_df", "local_df", "pico_df", None),
568+
("pico_df", "local_df", "cluster_df", "cloud_df", "Cluster"),
569+
("cloud_df", "local_df", "cluster_df", "pico_df", "Cluster"),
570+
("cloud_df", "cluster_df", "local_df", "pico_df", "Cluster"),
578571
("cloud_df", "cloud_df", "local_df", "pico_df", "Cloud"),
579572
# Still move everything to cloud
580573
("pico_df", "pico_df", "pico_df", "cloud_df", "Cloud"),
@@ -769,6 +762,19 @@ def test_switch_local_to_cloud_with_iloc___setitem__(local_df, cloud_df, pin_loc
769762
assert local_df.get_backend() == "Local_Machine" if pin_local else "Cloud"
770763

771764

765+
# This test should force the creation of a dataframe which
766+
# is too large for the backend and verify that it stays there
767+
# because there are no other options
768+
def test_single_backend_merge_no_good_options():
769+
with backend_test_context(
770+
test_backend="Small_Data_Local",
771+
choices=["Small_Data_Local"],
772+
):
773+
df1 = pd.DataFrame({"a": [1] * 100})
774+
df1["two"] = pd.to_datetime(df1["a"])
775+
assert df1.get_backend() == "Small_Data_Local"
776+
777+
772778
def test_stay_or_move_evaluation(cloud_high_self_df, default_df):
773779
default_cls = type(default_df._get_query_compiler())
774780
cloud_cls = type(cloud_high_self_df._get_query_compiler())

0 commit comments

Comments
 (0)