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

Introduce skip flag for preferential optimization #581

Merged
merged 9 commits into from
Aug 29, 2023

Conversation

moririn2528
Copy link
Contributor

Contributor License Agreement

This repository (optuna-dashboard) and Goptuna share common code.
This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Comment on lines 269 to 274
skiped_numbers = get_skiped_trials(study_id, storage)
return [
copy.deepcopy(t)
for t in ready_trials
if t.number not in worse_numbers and t.number not in skiped_numbers
]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skiped_numbers = get_skiped_trials(study_id, storage)
return [
copy.deepcopy(t)
for t in ready_trials
if t.number not in worse_numbers and t.number not in skiped_numbers
]
skipped_numbers = get_skipped_trials(study_id, storage)
return [
copy.deepcopy(t)
for t in ready_trials
if t.number not in worse_numbers and t.number not in skipped_numbers
]

Typo.

Comment on lines 63 to 75
def get_skiped_trials(
study_id: int,
storage: BaseStorage,
) -> list[int]:
"""Get trial numbers that have skip flag."""
skiped_trials: list[int] = []
summary = get_study_summary(storage, study_id)
system_attrs = getattr(summary, "system_attrs", {})
for k, v in system_attrs.items():
if not k.startswith(_SYSTEM_ATTR_PREFIX_SKIP_TRIAL):
continue
skiped_trials.append(int(k.split(":")[-1]))
return skiped_trials
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_skiped_trials(
study_id: int,
storage: BaseStorage,
) -> list[int]:
"""Get trial numbers that have skip flag."""
skiped_trials: list[int] = []
summary = get_study_summary(storage, study_id)
system_attrs = getattr(summary, "system_attrs", {})
for k, v in system_attrs.items():
if not k.startswith(_SYSTEM_ATTR_PREFIX_SKIP_TRIAL):
continue
skiped_trials.append(int(k.split(":")[-1]))
return skiped_trials
def get_skipped_trials(
study_id: int,
storage: BaseStorage,
) -> list[int]:
"""Get trial numbers that have skip flag."""
skipped_trials: list[int] = []
summary = get_study_summary(storage, study_id)
system_attrs = getattr(summary, "system_attrs", {})
for k, v in system_attrs.items():
if not k.startswith(_SYSTEM_ATTR_PREFIX_SKIP_TRIAL):
continue
skipped_trials.append(int(k.split(":")[-1]))
return skipped_trials

Typo.

@moririn2528 moririn2528 marked this pull request as ready for review August 28, 2023 10:04
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

@moririn2528 Thank you for your pull request!
Looks almost good to me. I left some suggestions though.

Comment on lines 271 to 277
for t in ready_trials:
if t.number in worse_numbers:
continue
skipped_key = _SYSTEM_ATTR_PREFIX_SKIP_TRIAL + str(t._trial_id)
if skipped_key in study_system_attrs:
continue
best_trials.append(copy.deepcopy(t))
Copy link
Member

Choose a reason for hiding this comment

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

I've just noticed that we do not need to declare ready_trials.

Suggested change
for t in ready_trials:
if t.number in worse_numbers:
continue
skipped_key = _SYSTEM_ATTR_PREFIX_SKIP_TRIAL + str(t._trial_id)
if skipped_key in study_system_attrs:
continue
best_trials.append(copy.deepcopy(t))
for t in storage.get_all_trials(study_id, deepcopy=False, states=(TrialState.COMPLETE, TrialState.RUNNING)):
if not t.system_attrs.get(_SYSTEM_ATTR_COMPARISON_READY, False):
continue
if t.number in worse_numbers:
continue
skipped_key = _SYSTEM_ATTR_PREFIX_SKIP_TRIAL + str(t._trial_id)
if skipped_key in study_system_attrs:
continue
best_trials.append(copy.deepcopy(t))

Copy link
Member

Choose a reason for hiding this comment

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

Also perhaps, it might be clearer to define def is_skipped_trial(trial_id: int) -> bool in optuna_dashboard/preferential/_system_attrs.py.

Comment on lines 338 to 342
summary = get_study_summary(storage, study_id)
if summary is None:
response.status = 404 # Not found
return {"reason": f"study_id={study_id} is not found"}
system_attrs = getattr(summary, "system_attrs", {})
Copy link
Member

Choose a reason for hiding this comment

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

Here, you can simply call get_study_system_attrs() storage api.
https://github.com/optuna/optuna/blob/ddfcafd796eb7f933438510fdc3c844864933fe9/optuna/storages/_base.py#L205

Suggested change
summary = get_study_summary(storage, study_id)
if summary is None:
response.status = 404 # Not found
return {"reason": f"study_id={study_id} is not found"}
system_attrs = getattr(summary, "system_attrs", {})
try:
system_attrs = storage.get_study_system_attrs(study_id)
except KeyError:
response.status = 404 # Not found
return {"reason": f"study_id={study_id} is not found"}

Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM after passing CI!

@c-bata c-bata changed the title add skip flag Introduce skip flag for preferential optimization Aug 29, 2023
@c-bata c-bata merged commit 6e490f8 into optuna:main Aug 29, 2023
11 checks passed
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.

3 participants