Fix storage API failing to return DF for experiments without any tunables

[eujing]

Pull Request

Fix storage API failing to return DF for experiments without any tunables

Addresses #884

---

Description

When an experiment is run without any tunables (benchmarking with default parameters), the storage API fails to return a dataframe of the results.

Example error:

>>> exp = storage.experiments["eujingchua-bench-57-02"]
>>> exp.results_df
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/workspaces/MySQL-Autotuning/MLOS/mlos_bench/mlos_bench/storage/sql/experiment_data.py", line 212, in results_df
    return common.get_results_df(self._engine, self._schema, self._experiment_id)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/MySQL-Autotuning/MLOS/mlos_bench/mlos_bench/storage/sql/common.py", line 183, in get_results_df
    ExperimentData.CONFIG_COLUMN_PREFIX + row.param_id,
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
TypeError: can only concatenate str (not "NoneType") to str

Going to the relevant line and inserting a breakpoint, the relevant columns are ["trial_id", "tunable_config_id", "param", "value"].
We can see the data is full of NULLs / Nones

(Pdb) results = configs.fetchall()
(Pdb) results
[(1, 6541, None, None), (2, 6541, None, None), (3, 6541, None, None), (4, 6541, None, None), (5, 6541, None, None), (6, 6541, None, None), (7, 6541, None, None), (8, 6541, None, None), (9, 6541, None, None), (10, 6541, None, None), (11, 6541, None, None), (12, 6541, None, None), (13, 6541, None, None), (14, 6541, None, None), (15, 6541, None, None), (16, 6541, None, None), (17, 6541, None, None), (18, 6541, None, None), (19, 6541, None, None), (20, 6541, None, None), (21, 6541, None, None), (22, 6541, None, None), (23, 6541, None, None), (24, 6541, None, None), (25, 6541, None, None), (26, 6541, None, None), (27, 6541, None, None), (28, 6541, None, None), (29, 6541, None, None), (30, 6541, None, None), (31, 6541, None, None), (32, 6541, None, None), (33, 6541, None, None), (34, 6541, None, None), (35, 6541, None, None), (36, 6541, None, None), (37, 6541, None, None), (38, 6541, None, None), (39, 6541, None, None), (40, 6541, None, None), (41, 6541, None, None), (42, 6541, None, None), (43, 6541, None, None), (44, 6541, None, None), (45, 6541, None, None), (46, 6541, None, None), (47, 6541, None, None), (48, 6541, None, None), (49, 6541, None, None), (50, 6541, None, None), (51, 6541, None, None), (52, 6541, None, None), (53, 6541, None, None), (54, 6541, None, None), (55, 6541, None, None), (56, 6541, None, None), (57, 6541, None, None), (58, 6541, None, None), (59, 6541, None, None), (60, 6541, None, None), (61, 6541, None, None), (62, 6541, None, None), (63, 6541, None, None), (64, 6541, None, None), (65, 6541, None, None), (66, 6541, None, None), (67, 6541, None, None), (68, 6541, None, None), (69, 6541, None, None), (70, 6541, None, None), (71, 6541, None, None), (72, 6541, None, None), (73, 6541, None, None), (74, 6541, None, None), (75, 6541, None, None), (76, 6541, None, None), (77, 6541, None, None), (78, 6541, None, None), (79, 6541, None, None), (80, 6541, None, None), (81, 6541, None, None), (82, 6541, None, None), (83, 6541, None, None), (84, 6541, None, None), (85, 6541, None, None), (86, 6541, None, None), (87, 6541, None, None), (88, 6541, None, None), (89, 6541, None, None), (90, 6541, None, None), (91, 6541, None, None), (92, 6541, None, None), (93, 6541, None, None), (94, 6541, None, None), (95, 6541, None, None), (96, 6541, None, None), (97, 6541, None, None), (98, 6541, None, None), (99, 6541, None, None), (100, 6541, None, None)]

---

Type of Change

Indicate the type of change by choosing one (or more) of the following:

• 🛠️ Bug fix

---

Testing

Unit test added covering this case, and also manual testing.

---

Additional Notes (optional)

Add any additional context or information for reviewers.

---

[motus]

I think we can filter out those nulls at the query level instead of doing it on the client side. maybe all we have to do is remove that isouter=True parameter at line 167? Can you please check?

P.S. I am trying to understand why on Earth I made this join outer and I can't remember the reason :) Most likely that's some copy/paste artifact, but please double check that everything works when you make this a regular inner join

[bpkroth]

I think we can filter out those nulls at the query level instead of doing it on the client side. maybe all we have to do is remove that isouter=True parameter at line 167? Can you please check?

P.S. I am trying to understand why on Earth I made this join outer and I can't remember the reason :) Most likely that's some copy/paste artifact, but please double check that everything works when you make this a regular inner join

We can have benchmarks that have no Tunables associated with them, in which case I think we still want to return other aspects of the Experiment data.

[bpkroth]

I think we can filter out those nulls at the query level instead of doing it on the client side. maybe all we have to do is remove that isouter=True parameter at line 167? Can you please check?
P.S. I am trying to understand why on Earth I made this join outer and I can't remember the reason :) Most likely that's some copy/paste artifact, but please double check that everything works when you make this a regular inner join

We can have benchmarks that have no Tunables associated with them, in which case I think we still want to return other aspects of the Experiment data.

Also, in that case, I think we should only return one row per trial, so the filtering client side isn't really a problem.

[bpkroth]

I think we can filter out those nulls at the query level instead of doing it on the client side. maybe all we have to do is remove that isouter=True parameter at line 167? Can you please check?
P.S. I am trying to understand why on Earth I made this join outer and I can't remember the reason :) Most likely that's some copy/paste artifact, but please double check that everything works when you make this a regular inner join

We can have benchmarks that have no Tunables associated with them, in which case I think we still want to return other aspects of the Experiment data.

Also, in that case, I think we should only return one row per trial, so the filtering client side isn't really a problem.

Nevermind. Querying results is already a separate data frame that gets merged at the end of this function.

Given that, the only reason I can think of to do the filtering client side is if we want the results_df property to also return data about Trials that have no results (e.g., they're still pending and haven't been evaluated yet), but I'm not sure what the point would be then.

@motus, thoughts?

[bpkroth]

I think we can filter out those nulls at the query level instead of doing it on the client side. maybe all we have to do is remove that isouter=True parameter at line 167? Can you please check?
P.S. I am trying to understand why on Earth I made this join outer and I can't remember the reason :) Most likely that's some copy/paste artifact, but please double check that everything works when you make this a regular inner join

We can have benchmarks that have no Tunables associated with them, in which case I think we still want to return other aspects of the Experiment data.

Also, in that case, I think we should only return one row per trial, so the filtering client side isn't really a problem.

Nevermind. Querying results is already a separate data frame that gets merged at the end of this function.

Given that, the only reason I can think of to do the filtering client side is if we want the results_df property to also return data about Trials that have no results (e.g., they're still pending and haven't been evaluated yet), but I'm not sure what the point would be then.

@motus, thoughts?

The more I've looked at this the more I think it should just be an inner join.

trials_df should already have rows for PENDING operations.
configs_df should be empty so that when it's merged with trials_df at the end, it's basically a no-op
results_df (if empty for pending results) should outer join in the merge as NULLs to indicate that there are no results, or else include all the results for that trial

[bpkroth]

One small suggested tweak to the test. Otherwise LGTM. Thanks!

[bpkroth]

Per the discussions, can you please also add a test to check that pending trials that don't yet have results, still show up in the results_df, with empty results columns?

[motus]

we can also add some failed trials for good measure

[motus]

Looks good! Thanks for fixing the bug, and double thanks for adding a unit test!

[bpkroth]

[like] Brian Kroth (GSL) reacted to your message:

…

________________________________ From: Sergiy Matusevych ***@***.***> Sent: Wednesday, December 4, 2024 11:33:56 PM To: microsoft/MLOS ***@***.***> Cc: Brian Kroth (GSL) ***@***.***>; Review requested ***@***.***> Subject: Re: [microsoft/MLOS] Fix storage API failing to return DF for experiments without any tunables (PR #889) @motus commented on this pull request.

________________________________ On mlos_bench/mlos_bench/tests/storage/exp_data_test.py<#889 (comment)>: we can also add some failed trials for good measure — Reply to this email directly, view it on GitHub<#889 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABQ53FAX7UPCVNGKVJ3X3A32D6GOJAVCNFSM6AAAAABS66R47OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIOBQGEZDINRSGU>. You are receiving this because your review was requested.Message ID: ***@***.***>