Fix storage API failing to return DF for experiments without any tunables by eujing · Pull Request #889 · microsoft/MLOS

eujing · 2024-12-04T00:43:42Z

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 · 2024-12-04T19:10:21Z

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 · 2024-12-04T19:15:05Z

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 · 2024-12-04T19:24:29Z

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 · 2024-12-04T20:53:38Z

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 · 2024-12-04T23:15:54Z

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?

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 · 2024-12-05T01:15:24Z

[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: ***@***.***>

Initial fix

d390f24

eujing requested a review from a team as a code owner December 4, 2024 00:43

Merge branch 'main' into eujingchua/fix-empty-tunables

6a49e8f

motus enabled auto-merge (squash) December 4, 2024 00:53

motus requested changes Dec 4, 2024

View reviewed changes

bpkroth reviewed Dec 4, 2024

View reviewed changes

Comment thread mlos_bench/mlos_bench/storage/sql/common.py

bpkroth disabled auto-merge December 4, 2024 20:54

Eu Jing Chua added 2 commits December 4, 2024 22:03

Change to inner join instead

2cfbf8d

Add unit test

a9710cc

eujing requested review from bpkroth and motus December 4, 2024 22:04

bpkroth reviewed Dec 4, 2024

View reviewed changes

Comment thread mlos_bench/mlos_bench/tests/storage/exp_data_test.py

bpkroth approved these changes Dec 4, 2024

View reviewed changes

Eu Jing Chua added 2 commits December 4, 2024 23:02

Address linting

74e6186

Additional check in test for no config columns

5bf80fe

bpkroth reviewed Dec 4, 2024

View reviewed changes

motus approved these changes Dec 4, 2024

View reviewed changes

bpkroth added the ready for review Ready for review label Dec 5, 2024

bpkroth merged commit c9fa067 into microsoft:main Dec 5, 2024

bpkroth mentioned this pull request Dec 5, 2024

Add additional tests for results_df with PENDING trials #892

Open

eujing mentioned this pull request Dec 5, 2024

Restoring past trials from storage fails when experiment has no tunables #884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Fix storage API failing to return DF for experiments without any tunables#889
bpkroth merged 6 commits into
microsoft:mainfrom
eujing:eujingchua/fix-empty-tunables

eujing commented Dec 4, 2024 •

edited

Loading

Uh oh!

motus left a comment

Uh oh!

bpkroth commented Dec 4, 2024

Uh oh!

Uh oh!

bpkroth commented Dec 4, 2024

Uh oh!

bpkroth commented Dec 4, 2024

Uh oh!

bpkroth commented Dec 4, 2024

Uh oh!

Uh oh!

bpkroth left a comment

Uh oh!

bpkroth Dec 4, 2024

Uh oh!

motus Dec 4, 2024

Uh oh!

motus left a comment

Uh oh!

bpkroth commented Dec 5, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

eujing commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request