From c27d50d007de1179b0b4d8f0098b999d39b2b938 Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Sun, 21 Jan 2024 12:59:48 +0100 Subject: [PATCH 1/9] tests: split test into multiple tests --- tests/ludwig/features/test_feature_utils.py | 61 ++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/tests/ludwig/features/test_feature_utils.py b/tests/ludwig/features/test_feature_utils.py index c0fd14ac9cd..4e45bcbc2ac 100644 --- a/tests/ludwig/features/test_feature_utils.py +++ b/tests/ludwig/features/test_feature_utils.py @@ -5,6 +5,65 @@ from ludwig.features import feature_utils +@pytest.fixture +def to_module() -> torch.nn.Module: + return torch.nn.Module() + + +@pytest.fixture +def type_module() -> torch.nn.Module: + return torch.nn.Module() + + +@pytest.fixture +def feature_dict(to_module: torch.nn.Module, type_module: torch.nn.Module) -> feature_utils.LudwigFeatureDict: + fdict = feature_utils.LudwigFeatureDict() + fdict.set("to", to_module) + fdict.set("type", type_module) + return fdict + + +def test_ludwig_feature_dict_get( + feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module +): + assert feature_dict.get("to") == to_module + assert feature_dict.get("type") == type_module + + +def test_ludwig_feature_dict_keys(feature_dict: feature_utils.LudwigFeatureDict): + assert feature_dict.keys() == ["to", "type"] + + +def test_ludwig_feature_dict_values( + feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module +): + assert list(feature_dict.values()) == [to_module, type_module] + + +def test_ludwig_feature_dict_items( + feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module +): + assert feature_dict.items() == [("to", to_module), ("type", type_module)] + + +def test_ludwig_feature_dict_iter(feature_dict: feature_utils.LudwigFeatureDict): + assert list(iter(feature_dict)) == ["to", "type"] + assert list(feature_dict) == ["to", "type"] + + +def test_ludwig_feature_dict_len(feature_dict: feature_utils.LudwigFeatureDict): + assert len(feature_dict) == 2 + + +def test_ludwig_feature_dict_update( + feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module +): + feature_dict.update({"to": torch.nn.Module(), "new": torch.nn.Module()}) + assert len(feature_dict) == 3 + assert not feature_dict.get("to") == to_module + assert feature_dict.get("type") == type_module + + def test_ludwig_feature_dict(): feature_dict = feature_utils.LudwigFeatureDict() @@ -15,7 +74,7 @@ def test_ludwig_feature_dict(): feature_dict.set("type", type_module) assert iter(feature_dict) is not None - assert next(feature_dict) is not None + # assert next(feature_dict) is not None assert len(feature_dict) == 2 assert feature_dict.keys() == ["to", "type"] assert feature_dict.items() == [("to", to_module), ("type", type_module)] From f8d7982ec4e265cdb243315828bb0d4bdc4affbd Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Sun, 21 Jan 2024 13:01:08 +0100 Subject: [PATCH 2/9] feat: improvate implementation of feature dict --- ludwig/features/feature_utils.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/ludwig/features/feature_utils.py b/ludwig/features/feature_utils.py index 50834a89cfc..7dcd03afc0f 100644 --- a/ludwig/features/feature_utils.py +++ b/ludwig/features/feature_utils.py @@ -14,7 +14,7 @@ # limitations under the License. # ============================================================================== import re -from typing import Dict, List, Optional, Tuple, Union +from typing import Dict, Iterator, List, Optional, Tuple, Union import numpy as np import torch @@ -174,33 +174,25 @@ class LudwigFeatureDict(torch.nn.Module): def __init__(self): super().__init__() self.module_dict = torch.nn.ModuleDict() - self.internal_key_to_original_name_map = {} def get(self, key) -> torch.nn.Module: return self.module_dict[get_module_dict_key_from_name(key)] def set(self, key: str, module: torch.nn.Module) -> None: module_dict_key_name = get_module_dict_key_from_name(key) - self.internal_key_to_original_name_map[module_dict_key_name] = key self.module_dict[module_dict_key_name] = module def __len__(self) -> int: return len(self.module_dict) - def __next__(self) -> None: - return next(iter(self)) - - def __iter__(self) -> None: + def __iter__(self) -> Iterator[str]: return iter(self.keys()) def keys(self) -> List[str]: - return [ - get_name_from_module_dict_key(feature_name) - for feature_name in self.internal_key_to_original_name_map.keys() - ] + return [get_name_from_module_dict_key(feature_name) for feature_name in self.module_dict.keys()] def values(self) -> List[torch.nn.Module]: - return [module for _, module in self.module_dict.items()] + return list(self.module_dict.values()) def items(self) -> List[Tuple[str, torch.nn.Module]]: return [ From 9dc7ef8a13075a39caf44f45898c20a534fedefe Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Sun, 21 Jan 2024 14:26:43 +0100 Subject: [PATCH 3/9] feat: use MutableMapping of collections.abc for feature dict --- ludwig/explain/captum.py | 10 ++-- ludwig/explain/captum_ray.py | 10 ++-- ludwig/explain/gbm.py | 4 +- ludwig/features/feature_utils.py | 39 +++++++------- ludwig/models/ecd.py | 2 +- ludwig/models/gbm.py | 4 +- ludwig/models/llm.py | 11 ++-- ludwig/trainers/trainer_lightgbm.py | 12 ++--- tests/ludwig/features/test_feature_utils.py | 57 ++++++++++++++++++--- 9 files changed, 98 insertions(+), 51 deletions(-) diff --git a/ludwig/explain/captum.py b/ludwig/explain/captum.py index 081568e18f7..97baa5df520 100644 --- a/ludwig/explain/captum.py +++ b/ludwig/explain/captum.py @@ -216,13 +216,13 @@ def explain(self) -> ExplanationsResult: feat_to_token_attributions_global[feat_name] = token_attributions_global self.global_explanation.add( - input_features.keys(), total_attribution_global, feat_to_token_attributions_global + input_features.key_list(), total_attribution_global, feat_to_token_attributions_global ) for i, (feature_attributions, explanation) in enumerate(zip(total_attribution, self.row_explanations)): # Add the feature attributions to the explanation object for this row. explanation.add( - input_features.keys(), + input_features.key_list(), feature_attributions, {k: v[i] for k, v in feat_to_token_attributions.items()}, ) @@ -245,7 +245,7 @@ def explain(self) -> ExplanationsResult: } # Prepend the negative class to the list of label explanations. self.global_explanation.add( - input_features.keys(), negated_attributions, negated_token_attributions, prepend=True + input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True ) for explanation in self.row_explanations: @@ -257,7 +257,9 @@ def explain(self) -> ExplanationsResult: if fa.token_attributions is not None } # Prepend the negative class to the list of label explanations. - explanation.add(input_features.keys(), negated_attributions, negated_token_attributions, prepend=True) + explanation.add( + input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True + ) # TODO(travis): for force plots, need something similar to SHAP E[X] expected_values.append(0.0) diff --git a/ludwig/explain/captum_ray.py b/ludwig/explain/captum_ray.py index 21d15db3815..7c2cb68a5b1 100644 --- a/ludwig/explain/captum_ray.py +++ b/ludwig/explain/captum_ray.py @@ -115,13 +115,13 @@ def explain(self) -> ExplanationsResult: feat_to_token_attributions_global[feat_name] = token_attributions_global self.global_explanation.add( - input_features.keys(), total_attribution_global, feat_to_token_attributions_global + input_features.key_list(), total_attribution_global, feat_to_token_attributions_global ) for i, (feature_attributions, explanation) in enumerate(zip(total_attribution, self.row_explanations)): # Add the feature attributions to the explanation object for this row. explanation.add( - input_features.keys(), + input_features.key_list(), feature_attributions, {k: v[i] for k, v in feat_to_token_attributions.items()}, ) @@ -140,7 +140,7 @@ def explain(self) -> ExplanationsResult: } # Prepend the negative class to the list of label explanations. self.global_explanation.add( - input_features.keys(), negated_attributions, negated_token_attributions, prepend=True + input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True ) for explanation in self.row_explanations: @@ -152,7 +152,9 @@ def explain(self) -> ExplanationsResult: if fa.token_attributions is not None } # Prepend the negative class to the list of label explanations. - explanation.add(input_features.keys(), negated_attributions, negated_token_attributions, prepend=True) + explanation.add( + input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True + ) # TODO(travis): for force plots, need something similar to SHAP E[X] expected_values.append(0.0) diff --git a/ludwig/explain/gbm.py b/ludwig/explain/gbm.py index 0eb8c652ac1..cacdec718eb 100644 --- a/ludwig/explain/gbm.py +++ b/ludwig/explain/gbm.py @@ -55,11 +55,11 @@ def explain(self) -> ExplanationsResult: expected_values = [] for _ in range(self.vocab_size): - self.global_explanation.add(base_model.input_features.keys(), feat_imp) + self.global_explanation.add(base_model.input_features.key_list(), feat_imp) for explanation in self.row_explanations: # Add the feature attributions to the explanation object for this row. - explanation.add(base_model.input_features.keys(), feat_imp) + explanation.add(base_model.input_features.key_list(), feat_imp) # TODO: expected_values.append(0.0) diff --git a/ludwig/features/feature_utils.py b/ludwig/features/feature_utils.py index 7dcd03afc0f..9b51406f7c2 100644 --- a/ludwig/features/feature_utils.py +++ b/ludwig/features/feature_utils.py @@ -14,7 +14,8 @@ # limitations under the License. # ============================================================================== import re -from typing import Dict, Iterator, List, Optional, Tuple, Union +from collections.abc import MutableMapping +from typing import Iterator, List, Optional, Tuple, Union import numpy as np import torch @@ -157,7 +158,7 @@ def get_name_from_module_dict_key(key: str, feature_name_suffix_length: int = FE return name[:-feature_name_suffix_length] -class LudwigFeatureDict(torch.nn.Module): +class LudwigFeatureDict(torch.nn.Module, MutableMapping): """Torch ModuleDict wrapper that permits keys with any name. Torch's ModuleDict implementation doesn't allow certain keys to be used if they conflict with existing class @@ -175,30 +176,30 @@ def __init__(self): super().__init__() self.module_dict = torch.nn.ModuleDict() - def get(self, key) -> torch.nn.Module: + def __getitem__(self, key: str) -> torch.nn.Module: return self.module_dict[get_module_dict_key_from_name(key)] - def set(self, key: str, module: torch.nn.Module) -> None: + def __setitem__(self, key: str, value: torch.nn.Module) -> None: module_dict_key_name = get_module_dict_key_from_name(key) - self.module_dict[module_dict_key_name] = module + self.module_dict[module_dict_key_name] = value - def __len__(self) -> int: - return len(self.module_dict) + def __delitem__(self, key: str) -> None: + del self.module_dict[get_module_dict_key_from_name(key)] def __iter__(self) -> Iterator[str]: - return iter(self.keys()) + return (get_name_from_module_dict_key(key) for key in self.module_dict) + + def __len__(self) -> int: + return len(self.module_dict) - def keys(self) -> List[str]: - return [get_name_from_module_dict_key(feature_name) for feature_name in self.module_dict.keys()] + def set(self, key: str, value: torch.nn.Module) -> None: + self[key] = value - def values(self) -> List[torch.nn.Module]: - return list(self.module_dict.values()) + def key_list(self) -> List[str]: + return list(self.keys()) - def items(self) -> List[Tuple[str, torch.nn.Module]]: - return [ - (get_name_from_module_dict_key(feature_name), module) for feature_name, module in self.module_dict.items() - ] + def value_list(self) -> List[torch.nn.Module]: + return list(self.values()) - def update(self, modules: Dict[str, torch.nn.Module]) -> None: - for feature_name, module in modules.items(): - self.set(feature_name, module) + def item_list(self) -> List[Tuple[str, torch.nn.Module]]: + return list(self.items()) diff --git a/ludwig/models/ecd.py b/ludwig/models/ecd.py index fcb5450d1cf..6b0cb64e01b 100644 --- a/ludwig/models/ecd.py +++ b/ludwig/models/ecd.py @@ -143,7 +143,7 @@ def forward( else: targets = None - assert list(inputs.keys()) == self.input_features.keys() + assert list(inputs.keys()) == list(self.input_features.keys()) encoder_outputs = self.encode(inputs) combiner_outputs = self.combine(encoder_outputs) diff --git a/ludwig/models/gbm.py b/ludwig/models/gbm.py index b249fce2cb6..a30c170025f 100644 --- a/ludwig/models/gbm.py +++ b/ludwig/models/gbm.py @@ -101,14 +101,14 @@ def forward( ) -> Dict[str, torch.Tensor]: # Invoke output features. output_logits = {} - output_feature_name = self.output_features.keys()[0] + output_feature_name = next(iter(self.output_features.keys())) output_feature = self.output_features.get(output_feature_name) # If `inputs` is a tuple, it should contain `(inputs, targets)`. if isinstance(inputs, tuple): inputs, _ = inputs - assert list(inputs.keys()) == self.input_features.keys() + assert list(inputs.keys()) == list(self.input_features.keys()) # If the model has not been compiled, predict using the LightGBM sklearn iterface. Otherwise, use torch with # the Hummingbird compiled model. Notably, when compiling the model to torchscript, compiling with Hummingbird diff --git a/ludwig/models/llm.py b/ludwig/models/llm.py index cc7cbc2efa8..a5d5da0639b 100644 --- a/ludwig/models/llm.py +++ b/ludwig/models/llm.py @@ -65,13 +65,13 @@ def __iter__(self) -> None: return iter(self.obj.keys()) def keys(self) -> List[str]: - return self.obj.keys() + return self.obj.key_list() def values(self) -> List[torch.nn.Module]: - return self.obj.values() + return self.obj.value_list() def items(self) -> List[Tuple[str, torch.nn.Module]]: - return self.obj.items() + return self.obj.item_list() def update(self, modules: Dict[str, torch.nn.Module]) -> None: self.obj.update(modules) @@ -148,7 +148,8 @@ def __init__( ) # Extract the decoder object for the forward pass - self._output_feature_decoder = ModuleWrapper(self.output_features.items()[0][1]) + decoder = next(iter(self.output_features.values())) + self._output_feature_decoder = ModuleWrapper(decoder) self.attention_masks = None @@ -401,7 +402,7 @@ def _unpack_inputs( else: targets = None - assert list(inputs.keys()) == self.input_features.keys() + assert list(inputs.keys()) == list(self.input_features.keys()) input_ids = self.get_input_ids(inputs) target_ids = self.get_target_ids(targets) if targets else None diff --git a/ludwig/trainers/trainer_lightgbm.py b/ludwig/trainers/trainer_lightgbm.py index 2a8174bc4f1..5e4b102d3a7 100644 --- a/ludwig/trainers/trainer_lightgbm.py +++ b/ludwig/trainers/trainer_lightgbm.py @@ -831,8 +831,8 @@ def _construct_lgb_datasets( validation_set: Optional["Dataset"] = None, # noqa: F821 test_set: Optional["Dataset"] = None, # noqa: F821 ) -> Tuple[lgb.Dataset, List[lgb.Dataset], List[str]]: - X_train = training_set.to_scalar_df(self.model.input_features.values()) - y_train = training_set.to_scalar_df(self.model.output_features.values()) + X_train = training_set.to_scalar_df(self.model.input_features.value_list()) + y_train = training_set.to_scalar_df(self.model.output_features.value_list()) # create dataset for lightgbm # keep raw data for continued training https://github.com/microsoft/LightGBM/issues/4965#issuecomment-1019344293 @@ -850,8 +850,8 @@ def _construct_lgb_datasets( eval_sets = [lgb_train] eval_names = [LightGBMTrainer.TRAIN_KEY] if validation_set is not None: - X_val = validation_set.to_scalar_df(self.model.input_features.values()) - y_val = validation_set.to_scalar_df(self.model.output_features.values()) + X_val = validation_set.to_scalar_df(self.model.input_features.value_list()) + y_val = validation_set.to_scalar_df(self.model.output_features.value_list()) try: lgb_val = lgb.Dataset(X_val, label=y_val, reference=lgb_train, free_raw_data=False).construct() except lgb.basic.LightGBMError as e: @@ -869,8 +869,8 @@ def _construct_lgb_datasets( pass if test_set is not None: - X_test = test_set.to_scalar_df(self.model.input_features.values()) - y_test = test_set.to_scalar_df(self.model.output_features.values()) + X_test = test_set.to_scalar_df(self.model.input_features.value_list()) + y_test = test_set.to_scalar_df(self.model.output_features.value_list()) try: lgb_test = lgb.Dataset(X_test, label=y_test, reference=lgb_train, free_raw_data=False).construct() except lgb.basic.LightGBMError as e: diff --git a/tests/ludwig/features/test_feature_utils.py b/tests/ludwig/features/test_feature_utils.py index 4e45bcbc2ac..db3c0a64af4 100644 --- a/tests/ludwig/features/test_feature_utils.py +++ b/tests/ludwig/features/test_feature_utils.py @@ -19,31 +19,35 @@ def type_module() -> torch.nn.Module: def feature_dict(to_module: torch.nn.Module, type_module: torch.nn.Module) -> feature_utils.LudwigFeatureDict: fdict = feature_utils.LudwigFeatureDict() fdict.set("to", to_module) - fdict.set("type", type_module) + fdict["type"] = type_module return fdict def test_ludwig_feature_dict_get( feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module ): - assert feature_dict.get("to") == to_module + assert feature_dict["to"] == to_module assert feature_dict.get("type") == type_module + assert feature_dict.get("other_key", default=None) is None def test_ludwig_feature_dict_keys(feature_dict: feature_utils.LudwigFeatureDict): - assert feature_dict.keys() == ["to", "type"] + assert list(feature_dict.keys()) == ["to", "type"] + assert feature_dict.key_list() == ["to", "type"] def test_ludwig_feature_dict_values( feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module ): assert list(feature_dict.values()) == [to_module, type_module] + assert feature_dict.value_list() == [to_module, type_module] def test_ludwig_feature_dict_items( feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module ): - assert feature_dict.items() == [("to", to_module), ("type", type_module)] + assert list(feature_dict.items()) == [("to", to_module), ("type", type_module)] + assert feature_dict.item_list() == [("to", to_module), ("type", type_module)] def test_ludwig_feature_dict_iter(feature_dict: feature_utils.LudwigFeatureDict): @@ -55,6 +59,17 @@ def test_ludwig_feature_dict_len(feature_dict: feature_utils.LudwigFeatureDict): assert len(feature_dict) == 2 +def test_ludwig_feature_dict_contains(feature_dict: feature_utils.LudwigFeatureDict): + assert "to" in feature_dict and "type" in feature_dict + + +def test_ludwig_feature_dict_eq(feature_dict: feature_utils.LudwigFeatureDict): + other_dict = feature_utils.LudwigFeatureDict() + assert not feature_dict == other_dict + other_dict.update(feature_dict.item_list()) + assert feature_dict == other_dict + + def test_ludwig_feature_dict_update( feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module ): @@ -64,6 +79,32 @@ def test_ludwig_feature_dict_update( assert feature_dict.get("type") == type_module +def test_ludwig_feature_dict_del(feature_dict: feature_utils.LudwigFeatureDict): + del feature_dict["to"] + assert len(feature_dict) == 1 + + +def test_ludwig_feature_dict_clear(feature_dict: feature_utils.LudwigFeatureDict): + feature_dict.clear() + assert len(feature_dict) == 0 + + +def test_ludwig_feature_dict_pop(feature_dict: feature_utils.LudwigFeatureDict, type_module: torch.nn.Module): + assert feature_dict.pop("type") == type_module + assert len(feature_dict) == 1 + assert feature_dict.pop("type", default=None) is None + + +def test_ludwig_feature_dict_popitem(feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module): + assert feature_dict.popitem() == ("to", to_module) + assert len(feature_dict) == 1 + + +def test_ludwig_feature_dict_setdefault(feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module): + assert feature_dict.setdefault("to") == to_module + assert feature_dict.get("other_key") is None + + def test_ludwig_feature_dict(): feature_dict = feature_utils.LudwigFeatureDict() @@ -76,8 +117,8 @@ def test_ludwig_feature_dict(): assert iter(feature_dict) is not None # assert next(feature_dict) is not None assert len(feature_dict) == 2 - assert feature_dict.keys() == ["to", "type"] - assert feature_dict.items() == [("to", to_module), ("type", type_module)] + assert feature_dict.key_list() == ["to", "type"] + assert feature_dict.item_list() == [("to", to_module), ("type", type_module)] assert feature_dict.get("to"), to_module feature_dict.update({"to_empty": torch.nn.Module()}) @@ -93,8 +134,8 @@ def test_ludwig_feature_dict_with_periods(): feature_dict.set("to.", to_module) - assert feature_dict.keys() == ["to."] - assert feature_dict.items() == [("to.", to_module)] + assert feature_dict.key_list() == ["to."] + assert feature_dict.item_list() == [("to.", to_module)] assert feature_dict.get("to.") == to_module From 4ca184aa98241d394bcba8303bc188cc98f84ff8 Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Sun, 21 Jan 2024 14:50:51 +0100 Subject: [PATCH 4/9] tests: add tests for the key-name mapping methods --- tests/ludwig/features/test_feature_utils.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/ludwig/features/test_feature_utils.py b/tests/ludwig/features/test_feature_utils.py index db3c0a64af4..a013689d8bd 100644 --- a/tests/ludwig/features/test_feature_utils.py +++ b/tests/ludwig/features/test_feature_utils.py @@ -105,6 +105,14 @@ def test_ludwig_feature_dict_setdefault(feature_dict: feature_utils.LudwigFeatur assert feature_dict.get("other_key") is None +@pytest.mark.parametrize("name", ["to", "type", "foo", "foo.bar"]) +def test_name_to_module_dict_key(name: str): + key = feature_utils.get_module_dict_key_from_name(name) + assert key != name + assert "." not in key + assert feature_utils.get_name_from_module_dict_key(key) == name + + def test_ludwig_feature_dict(): feature_dict = feature_utils.LudwigFeatureDict() From c5dcda9c9b0338f59df51d10be04157b702a6653 Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Mon, 22 Jan 2024 14:16:28 +0100 Subject: [PATCH 5/9] feat: use static hash value for feature dict --- ludwig/features/feature_utils.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ludwig/features/feature_utils.py b/ludwig/features/feature_utils.py index 9b51406f7c2..1ca1978ed33 100644 --- a/ludwig/features/feature_utils.py +++ b/ludwig/features/feature_utils.py @@ -203,3 +203,6 @@ def value_list(self) -> List[torch.nn.Module]: def item_list(self) -> List[Tuple[str, torch.nn.Module]]: return list(self.items()) + + def __hash__(self) -> int: + return 1 From 3edaa086ef87d7da8dea2bdd8c363a256e78904e Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Mon, 22 Jan 2024 14:55:05 +0100 Subject: [PATCH 6/9] refactor: remove list methods --- ludwig/explain/captum.py | 8 ++++---- ludwig/explain/captum_ray.py | 8 ++++---- ludwig/explain/gbm.py | 4 ++-- ludwig/features/feature_utils.py | 11 +---------- ludwig/models/llm.py | 6 +++--- ludwig/trainers/trainer_lightgbm.py | 12 ++++++------ tests/ludwig/features/test_feature_utils.py | 13 +++++-------- 7 files changed, 25 insertions(+), 37 deletions(-) diff --git a/ludwig/explain/captum.py b/ludwig/explain/captum.py index 97baa5df520..36cd0a569a9 100644 --- a/ludwig/explain/captum.py +++ b/ludwig/explain/captum.py @@ -216,13 +216,13 @@ def explain(self) -> ExplanationsResult: feat_to_token_attributions_global[feat_name] = token_attributions_global self.global_explanation.add( - input_features.key_list(), total_attribution_global, feat_to_token_attributions_global + list(input_features.keys()), total_attribution_global, feat_to_token_attributions_global ) for i, (feature_attributions, explanation) in enumerate(zip(total_attribution, self.row_explanations)): # Add the feature attributions to the explanation object for this row. explanation.add( - input_features.key_list(), + list(input_features.keys()), feature_attributions, {k: v[i] for k, v in feat_to_token_attributions.items()}, ) @@ -245,7 +245,7 @@ def explain(self) -> ExplanationsResult: } # Prepend the negative class to the list of label explanations. self.global_explanation.add( - input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True + list(input_features.keys()), negated_attributions, negated_token_attributions, prepend=True ) for explanation in self.row_explanations: @@ -258,7 +258,7 @@ def explain(self) -> ExplanationsResult: } # Prepend the negative class to the list of label explanations. explanation.add( - input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True + list(input_features.keys()), negated_attributions, negated_token_attributions, prepend=True ) # TODO(travis): for force plots, need something similar to SHAP E[X] diff --git a/ludwig/explain/captum_ray.py b/ludwig/explain/captum_ray.py index 7c2cb68a5b1..24e96a8c45d 100644 --- a/ludwig/explain/captum_ray.py +++ b/ludwig/explain/captum_ray.py @@ -115,13 +115,13 @@ def explain(self) -> ExplanationsResult: feat_to_token_attributions_global[feat_name] = token_attributions_global self.global_explanation.add( - input_features.key_list(), total_attribution_global, feat_to_token_attributions_global + list(input_features.keys()), total_attribution_global, feat_to_token_attributions_global ) for i, (feature_attributions, explanation) in enumerate(zip(total_attribution, self.row_explanations)): # Add the feature attributions to the explanation object for this row. explanation.add( - input_features.key_list(), + list(input_features.keys()), feature_attributions, {k: v[i] for k, v in feat_to_token_attributions.items()}, ) @@ -140,7 +140,7 @@ def explain(self) -> ExplanationsResult: } # Prepend the negative class to the list of label explanations. self.global_explanation.add( - input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True + list(input_features.keys()), negated_attributions, negated_token_attributions, prepend=True ) for explanation in self.row_explanations: @@ -153,7 +153,7 @@ def explain(self) -> ExplanationsResult: } # Prepend the negative class to the list of label explanations. explanation.add( - input_features.key_list(), negated_attributions, negated_token_attributions, prepend=True + list(input_features.keys()), negated_attributions, negated_token_attributions, prepend=True ) # TODO(travis): for force plots, need something similar to SHAP E[X] diff --git a/ludwig/explain/gbm.py b/ludwig/explain/gbm.py index cacdec718eb..395a596d2cc 100644 --- a/ludwig/explain/gbm.py +++ b/ludwig/explain/gbm.py @@ -55,11 +55,11 @@ def explain(self) -> ExplanationsResult: expected_values = [] for _ in range(self.vocab_size): - self.global_explanation.add(base_model.input_features.key_list(), feat_imp) + self.global_explanation.add(list(base_model.input_features.keys()), feat_imp) for explanation in self.row_explanations: # Add the feature attributions to the explanation object for this row. - explanation.add(base_model.input_features.key_list(), feat_imp) + explanation.add(list(base_model.input_features.keys()), feat_imp) # TODO: expected_values.append(0.0) diff --git a/ludwig/features/feature_utils.py b/ludwig/features/feature_utils.py index 1ca1978ed33..976fc16106e 100644 --- a/ludwig/features/feature_utils.py +++ b/ludwig/features/feature_utils.py @@ -15,7 +15,7 @@ # ============================================================================== import re from collections.abc import MutableMapping -from typing import Iterator, List, Optional, Tuple, Union +from typing import Iterator, List, Optional, Union import numpy as np import torch @@ -195,14 +195,5 @@ def __len__(self) -> int: def set(self, key: str, value: torch.nn.Module) -> None: self[key] = value - def key_list(self) -> List[str]: - return list(self.keys()) - - def value_list(self) -> List[torch.nn.Module]: - return list(self.values()) - - def item_list(self) -> List[Tuple[str, torch.nn.Module]]: - return list(self.items()) - def __hash__(self) -> int: return 1 diff --git a/ludwig/models/llm.py b/ludwig/models/llm.py index a5d5da0639b..acef9f8e222 100644 --- a/ludwig/models/llm.py +++ b/ludwig/models/llm.py @@ -65,13 +65,13 @@ def __iter__(self) -> None: return iter(self.obj.keys()) def keys(self) -> List[str]: - return self.obj.key_list() + return list(self.obj.keys()) def values(self) -> List[torch.nn.Module]: - return self.obj.value_list() + return list(self.obj.values()) def items(self) -> List[Tuple[str, torch.nn.Module]]: - return self.obj.item_list() + return list(self.obj.items()) def update(self, modules: Dict[str, torch.nn.Module]) -> None: self.obj.update(modules) diff --git a/ludwig/trainers/trainer_lightgbm.py b/ludwig/trainers/trainer_lightgbm.py index 5e4b102d3a7..01cd31082e3 100644 --- a/ludwig/trainers/trainer_lightgbm.py +++ b/ludwig/trainers/trainer_lightgbm.py @@ -831,8 +831,8 @@ def _construct_lgb_datasets( validation_set: Optional["Dataset"] = None, # noqa: F821 test_set: Optional["Dataset"] = None, # noqa: F821 ) -> Tuple[lgb.Dataset, List[lgb.Dataset], List[str]]: - X_train = training_set.to_scalar_df(self.model.input_features.value_list()) - y_train = training_set.to_scalar_df(self.model.output_features.value_list()) + X_train = training_set.to_scalar_df(list(self.model.input_features.values())) + y_train = training_set.to_scalar_df(list(self.model.output_features.values())) # create dataset for lightgbm # keep raw data for continued training https://github.com/microsoft/LightGBM/issues/4965#issuecomment-1019344293 @@ -850,8 +850,8 @@ def _construct_lgb_datasets( eval_sets = [lgb_train] eval_names = [LightGBMTrainer.TRAIN_KEY] if validation_set is not None: - X_val = validation_set.to_scalar_df(self.model.input_features.value_list()) - y_val = validation_set.to_scalar_df(self.model.output_features.value_list()) + X_val = validation_set.to_scalar_df(list(self.model.input_features.values())) + y_val = validation_set.to_scalar_df(list(self.model.output_features.values())) try: lgb_val = lgb.Dataset(X_val, label=y_val, reference=lgb_train, free_raw_data=False).construct() except lgb.basic.LightGBMError as e: @@ -869,8 +869,8 @@ def _construct_lgb_datasets( pass if test_set is not None: - X_test = test_set.to_scalar_df(self.model.input_features.value_list()) - y_test = test_set.to_scalar_df(self.model.output_features.value_list()) + X_test = test_set.to_scalar_df(list(self.model.input_features.values())) + y_test = test_set.to_scalar_df(list(self.model.output_features.values())) try: lgb_test = lgb.Dataset(X_test, label=y_test, reference=lgb_train, free_raw_data=False).construct() except lgb.basic.LightGBMError as e: diff --git a/tests/ludwig/features/test_feature_utils.py b/tests/ludwig/features/test_feature_utils.py index a013689d8bd..6f857b5e404 100644 --- a/tests/ludwig/features/test_feature_utils.py +++ b/tests/ludwig/features/test_feature_utils.py @@ -33,21 +33,18 @@ def test_ludwig_feature_dict_get( def test_ludwig_feature_dict_keys(feature_dict: feature_utils.LudwigFeatureDict): assert list(feature_dict.keys()) == ["to", "type"] - assert feature_dict.key_list() == ["to", "type"] def test_ludwig_feature_dict_values( feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module ): assert list(feature_dict.values()) == [to_module, type_module] - assert feature_dict.value_list() == [to_module, type_module] def test_ludwig_feature_dict_items( feature_dict: feature_utils.LudwigFeatureDict, to_module: torch.nn.Module, type_module: torch.nn.Module ): assert list(feature_dict.items()) == [("to", to_module), ("type", type_module)] - assert feature_dict.item_list() == [("to", to_module), ("type", type_module)] def test_ludwig_feature_dict_iter(feature_dict: feature_utils.LudwigFeatureDict): @@ -66,7 +63,7 @@ def test_ludwig_feature_dict_contains(feature_dict: feature_utils.LudwigFeatureD def test_ludwig_feature_dict_eq(feature_dict: feature_utils.LudwigFeatureDict): other_dict = feature_utils.LudwigFeatureDict() assert not feature_dict == other_dict - other_dict.update(feature_dict.item_list()) + other_dict.update(feature_dict.items()) assert feature_dict == other_dict @@ -125,8 +122,8 @@ def test_ludwig_feature_dict(): assert iter(feature_dict) is not None # assert next(feature_dict) is not None assert len(feature_dict) == 2 - assert feature_dict.key_list() == ["to", "type"] - assert feature_dict.item_list() == [("to", to_module), ("type", type_module)] + assert list(feature_dict.keys()) == ["to", "type"] + assert list(feature_dict.items()) == [("to", to_module), ("type", type_module)] assert feature_dict.get("to"), to_module feature_dict.update({"to_empty": torch.nn.Module()}) @@ -142,8 +139,8 @@ def test_ludwig_feature_dict_with_periods(): feature_dict.set("to.", to_module) - assert feature_dict.key_list() == ["to."] - assert feature_dict.item_list() == [("to.", to_module)] + assert list(feature_dict.keys()) == ["to."] + assert list(feature_dict.items()) == [("to.", to_module)] assert feature_dict.get("to.") == to_module From cd36fedfac5bc2e49304bcd97721681e7c85a8a4 Mon Sep 17 00:00:00 2001 From: Dennis Rall Date: Mon, 22 Jan 2024 18:20:58 +0100 Subject: [PATCH 7/9] docs: add docstrings to tests and to hash func --- ludwig/features/feature_utils.py | 1 + tests/ludwig/features/test_feature_utils.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/ludwig/features/feature_utils.py b/ludwig/features/feature_utils.py index 976fc16106e..ff134742a8e 100644 --- a/ludwig/features/feature_utils.py +++ b/ludwig/features/feature_utils.py @@ -196,4 +196,5 @@ def set(self, key: str, value: torch.nn.Module) -> None: self[key] = value def __hash__(self) -> int: + """Static hash value, because the object is mutable, but needs to be hashable for pytorch.""" return 1 diff --git a/tests/ludwig/features/test_feature_utils.py b/tests/ludwig/features/test_feature_utils.py index 6f857b5e404..5919e9b818a 100644 --- a/tests/ludwig/features/test_feature_utils.py +++ b/tests/ludwig/features/test_feature_utils.py @@ -7,11 +7,13 @@ @pytest.fixture def to_module() -> torch.nn.Module: + """Dummy Module to test the LudwigFeatureDict.""" return torch.nn.Module() @pytest.fixture def type_module() -> torch.nn.Module: + """Dummy Module to test the LudwigFeatureDict.""" return torch.nn.Module() From 326c40d1cb877afc57c812ddd9e6733e42d4a979 Mon Sep 17 00:00:00 2001 From: Justin Zhao Date: Tue, 23 Jan 2024 18:59:37 -0500 Subject: [PATCH 8/9] Upgrade integration tests to use torch 2.1.0. --- .github/workflows/pytest.yml | 2 +- tests/ludwig/features/test_feature_utils.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 813dc0b1076..7fab7c8e963 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -254,7 +254,7 @@ jobs: # remove torch and ray from the dependencies so we can add them depending on the matrix args for the job. cat requirements.txt | sed '/^torch[>=<\b]/d' | sed '/^torchtext/d' | sed '/^torchvision/d' | sed '/^torchaudio/d' > requirements-temp && mv requirements-temp requirements.txt cat requirements_distributed.txt | sed '/^ray[\[]/d' - pip install torch==2.0.0 torchtext torchvision torchaudio + pip install torch==2.1.0 torchtext torchvision torchaudio pip install ray==2.3.0 pip install '.[test]' pip list diff --git a/tests/ludwig/features/test_feature_utils.py b/tests/ludwig/features/test_feature_utils.py index 5919e9b818a..d59e2dea015 100644 --- a/tests/ludwig/features/test_feature_utils.py +++ b/tests/ludwig/features/test_feature_utils.py @@ -122,7 +122,6 @@ def test_ludwig_feature_dict(): feature_dict.set("type", type_module) assert iter(feature_dict) is not None - # assert next(feature_dict) is not None assert len(feature_dict) == 2 assert list(feature_dict.keys()) == ["to", "type"] assert list(feature_dict.items()) == [("to", to_module), ("type", type_module)] From 23a2da7ff7ce7fd93b9f67ee50e0bdf2db697f8d Mon Sep 17 00:00:00 2001 From: Justin Zhao Date: Mon, 29 Jan 2024 15:00:51 -0500 Subject: [PATCH 9/9] Try adding sox to requirements. --- requirements.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/requirements.txt b/requirements.txt index 1b2f320d387..8c58ec0875f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -67,3 +67,6 @@ datasets # pin required for torch 2.1.0 urllib3<2 + +# required for torchaudio 2.1.0 +sox