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

Add number of suggest() attempts in Algo wrapper #883

Merged
merged 9 commits into from
Apr 26, 2022
98 changes: 61 additions & 37 deletions src/orion/core/worker/primary_algo.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,48 +137,72 @@ def suggest(self, num: int) -> list[Trial] | None:
New parameters must be compliant with the problem's domain `orion.algo.space.Space`.

"""
transformed_trials = self.algorithm.suggest(num)

if transformed_trials is None:
return None

trials: list[Trial] = []
for transformed_trial in transformed_trials:
if transformed_trial not in self.transformed_space:
raise ValueError(
f"Trial {transformed_trial.id} not contained in space:\n"
f"Params: {transformed_trial.params}\n"
f"Space: {self.transformed_space}"
)
original = self.transformed_space.reverse(transformed_trial)
if original in self.registry:
max_suggest_attempts = 100
Copy link
Member

Choose a reason for hiding this comment

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

This should be configurable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a constructor argument to the wrapper? Or parameter to suggest ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made it an attribute in 872f094, is this sufficient, you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay. It's a bit tricky here since at the constructor level it would add an argument specific for the wrapper, should I don't think should be exposed to the users since the define the algo configuration directly, they are not even aware that there is a wrapper. Same for suggest, it should follow the same interface as the algorithms it is wrapping. Maybe your solution is the best one for now and we can reassess later on if needed.


# NOTE: Should we also do this kind of repeated checking until the algo produces `num`
bouthilx marked this conversation as resolved.
Show resolved Hide resolved
# trials?

for suggest_attempt in range(1, max_suggest_attempts + 1):
transformed_trials: list[Trial] | None = self.algorithm.suggest(num)
transformed_trials = transformed_trials or []

for transformed_trial in transformed_trials:
if transformed_trial not in self.transformed_space:
raise ValueError(
f"Trial {transformed_trial.id} not contained in space:\n"
f"Params: {transformed_trial.params}\n"
f"Space: {self.transformed_space}"
)
original = self.transformed_space.reverse(transformed_trial)
if original in self.registry:
logger.debug(
"Already have a trial that matches %s in the registry.",
original,
)
# We already have a trial that is equivalent to this one.
# Fetch the actual trial (with the status and possibly results)
original = self.registry.get_existing(original)
logger.debug("Matching trial (with results/status): %s", original)

# Copy over the status and results from the original to the transformed trial
# and observe it.
transformed_trial = _copy_status_and_results(
original_trial=original, transformed_trial=transformed_trial
)
logger.debug(
"Transformed trial (with results/status): %s", transformed_trial
)
self.algorithm.observe([transformed_trial])
else:
# We haven't seen this trial before. Register it.
self.registry.register(original)
trials.append(original)

# NOTE: Here we DON'T register the transformed trial, we let the algorithm do it
# itself in its `suggest`.
# Register the equivalence between these trials.
self.registry_mapping.register(original, transformed_trial)

if trials:
if suggest_attempt > 1:
logger.debug(
f"Succeeded in suggesting new trials after {suggest_attempt} attempts."
)
return trials

if self.is_done:
logger.debug(
"Already have a trial that matches %s in the registry.", original
f"Algorithm is done! (after {suggest_attempt} sampling attempts)."
)
# We already have a trial that is equivalent to this one.
# Fetch the actual trial (with the status and possibly results)
original = self.registry.get_existing(original)
logger.debug("Matching trial (with results/status): %s", original)
break

# Copy over the status and results from the original to the transformed trial
# and observe it.
transformed_trial = _copy_status_and_results(
original_trial=original, transformed_trial=transformed_trial
)
logger.debug(
"Transformed trial (with results/status): %s", transformed_trial
)
self.algorithm.observe([transformed_trial])
else:
# We haven't seen this trial before. Register it.
self.registry.register(original)
trials.append(original)

# NOTE: Here we DON'T register the transformed trial, we let the algorithm do it itself
# in its `suggest`.
# Register the equivalence between these trials.
self.registry_mapping.register(original, transformed_trial)
return trials
logger.warning(
f"Unable to sample a new trial from the algorithm, even after "
f"{max_suggest_attempts} attempts! Returning an empty list."
)
return []

def observe(self, trials: list[Trial]) -> None:
"""Observe evaluated trials.
Expand Down
12 changes: 10 additions & 2 deletions tests/unittests/core/test_primary_algo.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,16 @@ def __init__(self, space: Space, fixed_suggestion: Trial):
assert fixed_suggestion in space

def suggest(self, num):
self.register(self.fixed_suggestion)
return [self.fixed_suggestion]
# NOTE: can't register the trial if it's already here. The fixed suggestion is always "new",
# but the algorithm actually observes it at some point. Therefore, we don't overwrite what's
# already in the registry.
if not self.has_suggested(self.fixed_suggestion):
self.register(self.fixed_suggestion)
return [self.fixed_suggestion]
# TODO: Does it make sense for the algorithm to just not suggest anything, if it has already
bouthilx marked this conversation as resolved.
Show resolved Hide resolved
# suggested this one? Does this make sense? Or should it keep suggesting it over and over
# again?
return []


@pytest.fixture()
Expand Down