My fixes #815

yshady · 2024-07-24T01:15:52Z

how i ran experiments

…ptimizer to crash

bpkroth · 2024-07-24T19:30:14Z

mlos_bench/mlos_bench/optimizers/base_optimizer.py

@@ -76,7 +76,7 @@ def __init__(self,
        # if True (default), use the already initialized values for the first iteration.
        self._start_with_defaults: bool = bool(
            strtobool(str(self._config.pop('start_with_defaults', True))))
-        self._max_iter = int(self._config.pop('max_suggestions', 100))
+        self._max_iter = int(self._config.pop('max_suggestions', 5000))


Think we should drop this. It can be changed in configs.

bpkroth · 2024-07-24T19:30:37Z

mlos_bench/mlos_bench/optimizers/base_optimizer.py

+        _LOG.info(
+            "Update the optimizer with: %d configs, %d scores, %d status values",
+            len(configs or []), len(scores or []), len(status or [])
+        )


What's the change here?

none at all :) just format really

@yshady the general rule for submitting PRs (open-source or not) is to separate the functionality changes from cosmetic fixes and make sure each PR is as small as possible (but not smaller)

Yeah sorry this is kinda a first for me :) like a kid learning to walk for first time haha

let me try to clean this up tomorrow after my demo and explain some of the stuff. I also still need to figure out how to use GitHub correctly

bpkroth · 2024-07-24T19:32:32Z

mlos_bench/mlos_bench/optimizers/base_optimizer.py

+        if len(configs or []) == 1:
+            _LOG.info("Only one configuration provided, using defaults.")
+            self._start_with_defaults = True
+        elif has_data and self._start_with_defaults:


This seems like the meat of the change. Can you please update the PR description to explain this?

If there was no config it would freak out and say hey you have no trials in the db I cannot suggest a config based on nothing

bpkroth · 2024-07-24T19:33:03Z

mlos_bench/mlos_bench/optimizers/mlos_core_optimizer.py

-                      status: Optional[Sequence[Status]] = None) -> bool:
+                  configs: Sequence[dict],
+                  scores: Sequence[Optional[Dict[str, TunableValue]]],
+                  status: Optional[Sequence[Status]] = None) -> bool:


Please undo whitespace only changes. They make it harder to understand what's relevant. Thanks!

Yeah sorry I only use GitHub for my personal code, never work in teams using it so first time experience really. My school has decided that math is more important than GitHub experience

bpkroth · 2024-07-24T19:33:33Z

mlos_bench/mlos_bench/storage/sql/schema.py

@@ -47,7 +47,7 @@ class DbSchema:
    # pylint: disable=too-many-instance-attributes

    # Common string column sizes.
-    _ID_LEN = 512
+    _ID_LEN = 256


What's this change about?

some sql stuff me and Eu Jing both have this, I believe kelly too

I don't think we need that. I believe this is from the times @eujing was trying to work around my bug in MLOS when I was saving callables in the DB instead of actual values :)

Yes, @DelphianCalamity, @yshady and I have been needing this change whenever we use a fresh database and sqlalchemy is trying to create tables for the first time. @motus I think you did partially fix the original issue we had but, this column size is still too large for MySQL-backends.

Summarizing a thread we have in teams with Kelly, the current offending table is trial_param.
The DDL is roughly:

CREATE TABLE trial_param ( exp_id VARCHAR(512) NOT NULL, trial_id INTEGER NOT NULL, param_id VARCHAR(512) NOT NULL, param_value VARCHAR(1024), PRIMARY KEY (exp_id, trial_id, param_id), FOREIGN KEY (exp_id, trial_id) REFERENCES trial (exp_id, trial_id) )

The primary key becomes an issue when using standard encodings like utfmb3 (1-3 bytes per char) or utfmb4 (more bytes per char).
The upper bound for (varchar 512 + integer + varchar 512) in these encodings exceeds 3072 bytes (the limit for MySQL keys)

This results in the MySQL error Specified key was too long; max key length is 3072 bytes when executing DDL on a new database.

But I agree, these changes should be split up into individual PRs. I think Yaseen just wanted to store all his changes in a branch in-case.

Thanks for explaining @eujing

bpkroth · 2024-07-24T19:33:50Z

mlos_core/mlos_core/optimizers/bayesian_optimizers/smac_optimizer.py

@@ -35,7 +35,7 @@ def __init__(self, *,  # pylint: disable=too-many-locals,too-many-arguments
                 seed: Optional[int] = 0,
                 run_name: Optional[str] = None,
                 output_directory: Optional[str] = None,
-                 max_trials: int = 100,
+                 max_trials: int = 5000,


bpkroth · 2024-07-24T19:33:55Z

..._bench/mlos_bench/tests/config/schemas/schedulers/test-cases/good/full/sync_sched-full.jsonc

@@ -7,6 +7,6 @@
        "experiment_id": "MyExperimentName",
        "config_id": 1,
        "trial_id": 1,
-        "max_trials": 100
+        "max_trials": 200


bpkroth · 2024-07-24T19:34:19Z

mlos_bench/mlos_bench/optimizers/mlos_core_optimizer.py

@@ -104,6 +104,9 @@ def bulk_register(self,
        df_scores = self._adjust_signs_df(
            pd.DataFrame([{} if score is None else score for score in scores]))

+        # Convert all score columns to numeric, coercing errors to NaN
+        df_scores = df_scores.apply(pd.to_numeric, errors='coerce')


Was this fixed already in #789 ?

should be but I haven't tested

@bpkroth yes, this is more heavy-handed version of what we already have in master

bpkroth · 2024-07-24T19:35:12Z

mlos_bench/mlos_bench/optimizers/mlos_core_optimizer.py

+            if target in scores:
+                if pd.isna(scores[target]):
+                    _LOG.warning(f"'{target}' is NaN in the best observation. Setting it to 0.")
+                    scores[target] = 0


I think this should be optional behavior. It might be better to mark the Trial as FAILED instead and ask the user to check their scripts.

See also #523

I agree setting nan to 0 is not ideal but I desperately wanted long running experiments

I think it is totally fine to report NaNs here - say, for scores that were not the primary optimization target or at all absent in a particular trial (e.g., because of the experiments' merge). Not to mention that 0 may not be the right value to impute, depending on your optimization direction. I am afraid that @yshady is bending MLOS to make the UI work smoothly and I think it should be the other way around :)

@motus no the UI is separate from MLOS experiments. All it does is run the commands. Yes these fixes were to just make MLOS run for long periods of time and they are shortcuts and not ideal. @eujing can confirm as well, I am bending MLOS to just work :) not just for the UI but for the sake of collecting data

@motus also optimizer does not like nan values from my experience

I will try to impute the correct value tomorrow

bpkroth · 2024-07-24T19:36:01Z

how i ran experiments

Left some comments.

Can you please split this out and give it a more descriptive name/description?

motus · 2024-07-25T00:02:42Z

mlos_bench/mlos_bench/optimizers/mlos_core_optimizer.py

@@ -104,6 +104,9 @@ def bulk_register(self,
        df_scores = self._adjust_signs_df(
            pd.DataFrame([{} if score is None else score for score in scores]))

+        # Convert all score columns to numeric, coercing errors to NaN
+        df_scores = df_scores.apply(pd.to_numeric, errors='coerce')


@bpkroth yes, this is more heavy-handed version of what we already have in master

motus · 2024-07-25T00:08:12Z

mlos_bench/mlos_bench/optimizers/mlos_core_optimizer.py

+            if target in scores:
+                if pd.isna(scores[target]):
+                    _LOG.warning(f"'{target}' is NaN in the best observation. Setting it to 0.")
+                    scores[target] = 0


I think it is totally fine to report NaNs here - say, for scores that were not the primary optimization target or at all absent in a particular trial (e.g., because of the experiments' merge). Not to mention that 0 may not be the right value to impute, depending on your optimization direction. I am afraid that @yshady is bending MLOS to make the UI work smoothly and I think it should be the other way around :)

motus · 2024-07-25T00:09:46Z

mlos_bench/mlos_bench/storage/sql/schema.py

@@ -47,7 +47,7 @@ class DbSchema:
    # pylint: disable=too-many-instance-attributes

    # Common string column sizes.
-    _ID_LEN = 512
+    _ID_LEN = 256


I don't think we need that. I believe this is from the times @eujing was trying to work around my bug in MLOS when I was saving callables in the DB instead of actual values :)

Yaseen Shady added 3 commits July 5, 2024 18:58

Enforce float, avoid crashing optimizer. If value is NAN will cause o…

be04f60

…ptimizer to crash

Max iterations for SMAC set too low, increase limit

3cd495a

this is how I got experiments to run!

87edbcd

yshady requested a review from a team as a code owner July 24, 2024 01:15

bpkroth reviewed Jul 24, 2024

View reviewed changes

motus requested changes Jul 25, 2024

View reviewed changes

yshady closed this Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

My fixes #815

My fixes #815

yshady commented Jul 24, 2024

bpkroth Jul 24, 2024

bpkroth Jul 24, 2024

yshady Jul 24, 2024 •

edited

Loading

motus Jul 25, 2024

yshady Jul 25, 2024

yshady Jul 25, 2024

bpkroth Jul 24, 2024

yshady Jul 24, 2024

bpkroth Jul 24, 2024

yshady Jul 25, 2024

bpkroth Jul 24, 2024

yshady Jul 24, 2024

motus Jul 25, 2024

eujing Jul 25, 2024 •

edited

Loading

yshady Jul 25, 2024

bpkroth Jul 24, 2024

bpkroth Jul 24, 2024

bpkroth Jul 24, 2024

yshady Jul 24, 2024

motus Jul 25, 2024

bpkroth Jul 24, 2024

bpkroth Jul 24, 2024

yshady Jul 24, 2024

motus Jul 25, 2024

yshady Jul 25, 2024

yshady Jul 25, 2024

yshady Jul 25, 2024

bpkroth commented Jul 24, 2024

motus Jul 25, 2024

motus Jul 25, 2024

motus Jul 25, 2024

My fixes #815

My fixes #815

Conversation

yshady commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yshady Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eujing Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpkroth commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yshady Jul 24, 2024 •

edited

Loading

eujing Jul 25, 2024 •

edited

Loading