Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Metadata for SMAC enabling Multi-Fidelity #771

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

jsfreischuetz
Copy link
Contributor

@jsfreischuetz jsfreischuetz commented Jul 2, 2024

Enables changing the defaults in the SMAC optimizer to enable state passing (what SMAC internally calls context, but that doesn't match MLOS's notion of context) and multi-fidelity tuning.

This required a few additional changes to metadata throughout the project

Additionally, this PR includes a test case for Multi-Fidelity Tuning

This PR replaces the previous one (#751)

@jsfreischuetz jsfreischuetz marked this pull request as ready for review July 2, 2024 22:26
@jsfreischuetz jsfreischuetz requested a review from a team as a code owner July 2, 2024 22:26
.cspell.json Outdated Show resolved Hide resolved
self.trial_info_map: Dict[ConfigSpace.Configuration, TrialInfo] = {}
self.trial_info_df: pd.DataFrame = pd.DataFrame(
columns=["Configuration", "Metadata", "TrialInfo", "TrialValue"]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why replacing this with a dataframe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because configurations are no longer a unique identifier if we support metadata.

An alternative would be to use the type:

Dict[Tuple[ConfigSpace.Configuration, pd.DataFrame], Tuple[TrialInfo, TrialValue]]

but I thought using a dataframe was simpler and more flexible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not certain you can actually use that Tuple as a Dict key (may not be hashable).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tuples are hashable, but dataframes are not

)

self.lock = threading.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some comments regarding the need for the threading lock.

[df_ctx.equals(ctx) for df_ctx in self.trial_info_df["Metadata"]]
)

# make a new entry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments could use some improvement.

Make a new entry of what? What's the first part doing that this one now needs explanation?

for _, _, _, metadata in observations]).reset_index(drop=True)
return (configs, scores, contexts if len(contexts.columns) > 0 else None, metadatas if len(metadatas.columns) > 0 else None)

def get_observations(self) -> Tuple[pd.DataFrame, pd.DataFrame, Optional[pd.DataFrame], Optional[pd.DataFrame]]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These Tuples are getting a little large and hard to read (recall a previous version of this PR where the order of them was mistakenly swapped at one point).

Think we discussed creating a NamedTuple or small DataClass for them instead so that they can be accessed by name in order to make it more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want I can do this in this PR, or another follow up PR

Copy link
Contributor

@bpkroth bpkroth Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a predecessor PR would be better. Much like we did with adding the metadata args and named args first.

@pytest.mark.parametrize(('optimizer_type', 'verify', 'kwargs'), [
# Enumerate all supported Optimizers
*[(member, verify, {"seed": SEED, "facade": MFFacade, "intensifier": SuccessiveHalving, "min_budget": 1, "max_budget": 9})
for member, verify in [(OptimizerType.SMAC, smac_verify_best)]],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty weird pattern. If we wanted to add more types in the future those kwargs wouldn't work.

What we've done elsewhere is

  1. retain the loop over OptimizerType in order to make sure that all optimizers get a test added (or explicitly excluded) whenever we add new optimizer backends.
  2. When necessary, within the body of the test set additional parameters in a switch statement for a given optimizer type. e.g.,
if optimizer_type == OptimizerType.SMAC:
  verifier = smac_verify_best
elif optimizer_type == OptimizerType.FLAML:
  pytest.skip("TODO: FLAML Optimizer does not yet support metadata")  # though even here, I think we should test *something*
else:
  raise NotImplementedError(f"Missing test handler for OptimizerType {optimizer_type}")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the kwargs into the tuple which fixes the kwargs problem.

I find switching over unsupported optimizers messy since it means, in this case, we should skip the entire test as you have above, unless we are testing SMAC. There are other places where we test to make sure that using metadata throws exceptions, so without metadata support there is nothing to test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when we add that functionality to another optimizer it will be very easy to forget to enable related testing support, so I'd rather add that now as it will be easier to search for what we need to enable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it be the same either way? One requires adding a line, the other requires moving a line. I make this more similar to other tests, but I don't think it changes much either way.

jsfreischuetz and others added 24 commits July 8, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants