Disallow certain config parameters from accepting `null` as a value #3079

abidwael · 2023-02-11T07:27:14Z

No description provided.

github-actions · 2023-02-11T09:40:06Z

Unit Test Results

        6 files +    1         6 suites +1 5h 28m 7s ⏱️ + 1h 45m 46s
  3 930 tests ±    0   3 892 ✔️ -     1   37 💤 ±  0 1 ❌ +1
11 787 runs +202 11 675 ✔️ +177 111 💤 +24 1 ❌ +1

For more details on these failures, see this check.

Results for commit b0543ce. ± Comparison against base commit 1f83833.

♻️ This comment has been updated with latest results.

This reverts commit 999e4f1.

ludwig/schema/hyperopt/executor.py

ludwig/schema/utils.py

tgaddair

General feedback is that it's confusing to both have None be the default and allow_none=False. I would suggest either:

Making allow_none=True the default
Making default a required param

abidwael · 2023-02-13T08:18:18Z

Makes sense @tgaddair.

I made default a required param and kept allow_none=False so we don't run into the same problem when for other parameters in the config or when we add new parameters. I did this for most of the config parameter types (all except Dict, List, and DictList). I kept the rest as they are because it seems like lots of them (if not all) have default=None in the schema. These could be schemafied and that way the default values could live in the schema instead of the model initialization code.

tgaddair · 2023-02-14T20:01:31Z

ludwig/schema/utils.py

    description: str = "",
    max: Optional[float] = None,
    parameter_metadata: ParameterMetadata = None,
 ):
    """Returns a dataclass field with marshmallow metadata enforcing numeric inputs must be nonnegative."""
    val = validate.Range(min=0.0, max=max)
-    allow_none = allow_none or default is None
+    # allow_none = allow_none or default is None


Please remove commented code.

tgaddair · 2023-02-14T20:01:37Z

ludwig/schema/utils.py

@@ -605,7 +603,7 @@ def FloatRange(
    """Returns a dataclass field with marshmallow metadata enforcing numeric inputs must be in range set by
    relevant keyword args."""
    val = validate.Range(min=min, max=max, min_inclusive=min_inclusive, max_inclusive=max_inclusive)
-    allow_none = allow_none or default is None
+    # allow_none = allow_none or default is None


tgaddair · 2023-02-14T20:01:45Z

tests/integration_tests/test_model_training_options.py

-    assert train_losses[last_entry - 1] <= train_losses[0]
+    # ensure train loss for last entry is less than to the first entry.
+    np.testing.assert_array_less(train_losses[last_entry - 1], train_losses[0])
+    # assert train_losses[last_entry - 1] <= train_losses[0]


tgaddair

LGTM, only nit is to remove the dead code you commented out.

Wael Abid added 3 commits February 10, 2023 23:25

default to allow_none=False

5b8a6aa

expand

46ed80c

fix

7ad89f4

Wael Abid added 4 commits February 11, 2023 15:54

fix stringoption fields

999e4f1

Revert "fix stringoption fields"

5503ba5

This reverts commit 999e4f1.

fix stringoptions

f77be2e

fix test

bfab174