Ensure trial notes are transferred when renamed #703

adjeiv · 2023-11-18T11:03:32Z

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

Fixes #660

What does this implement/fix? Explain your changes.

This PR defines a few more functions in _note, to delete and transfer notes for a study/trial. Tests are also added, and I've tested this manually locally.

chrome_WvXYApKXjn.mp4

…o save_sys_attrs

codecov · 2023-11-18T11:06:53Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (462f609) 62.88% compared to head (caa2d0c) 65.80%.
Report is 80 commits behind head on main.

Files	Patch %	Lines
optuna_dashboard/_app.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #703      +/-   ##
==========================================
+ Coverage   62.88%   65.80%   +2.92%     
==========================================
  Files          35       35              
  Lines        2250     2293      +43     
==========================================
+ Hits         1415     1509      +94     
+ Misses        835      784      -51

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

c-bata

Thank you for your pull request. I left a comment.

optuna_dashboard/_note.py

adjeiv · 2023-11-27T17:36:04Z

Thank you for the swift review.
This was my initial approach; however, the keys used for the notes embed the trial id within them. So when we create the new study, we also get entirely new trial IDs, and so we wouldn't be able to find their notes.

Let me know if there's another method to work around this!

c-bata

Understood, you're right. Maybe the notes should have used the trial.number instead of the trial_id as the key, but maintaining backward compatibility with that change would be difficult. So I think it would be best to merge this PR first.

I left some review comments.

c-bata · 2023-11-28T03:00:12Z

optuna_dashboard/_note.py

+def transfer_notes(storage: BaseStorage, src_study: optuna.Study, dst_study: optuna.Study) -> None:
+    system_attrs = storage.get_study_system_attrs(study_id=src_study._study_id)
+
+    def transfer(src_trial_id: Optional[int], dst_trial_id: Optional[int]) -> None:
+        note = get_note_from_system_attrs(system_attrs, src_trial_id)["body"]
+        save_note_with_version(storage, dst_study._study_id, dst_trial_id, 0, note)
+        delete_notes(storage, src_study._study_id, src_trial_id)
+
+    # Transfer individual trial notes
+    for src_trial, dst_trial in zip(src_study.get_trials(), dst_study.get_trials()):
+        transfer(src_trial._trial_id, dst_trial._trial_id)
+
+    # Transfer study note
+    NO_SRC_TRIAL, NO_DST_TRIAL = None, None
+    transfer(NO_SRC_TRIAL, NO_DST_TRIAL)


I would say we don't need to delete original notes. How about making it copy instead of transfer since we can reduce the number of storage API calls?

Suggested change

def transfer_notes(storage: BaseStorage, src_study: optuna.Study, dst_study: optuna.Study) -> None:

system_attrs = storage.get_study_system_attrs(study_id=src_study._study_id)

def transfer(src_trial_id: Optional[int], dst_trial_id: Optional[int]) -> None:

note = get_note_from_system_attrs(system_attrs, src_trial_id)["body"]

save_note_with_version(storage, dst_study._study_id, dst_trial_id, 0, note)

delete_notes(storage, src_study._study_id, src_trial_id)

# Transfer individual trial notes

for src_trial, dst_trial in zip(src_study.get_trials(), dst_study.get_trials()):

transfer(src_trial._trial_id, dst_trial._trial_id)

# Transfer study note

NO_SRC_TRIAL, NO_DST_TRIAL = None, None

transfer(NO_SRC_TRIAL, NO_DST_TRIAL)

def copy_notes(storage: BaseStorage, src_study: optuna.Study, dst_study: optuna.Study) -> None:

system_attrs = storage.get_study_system_attrs(study_id=src_study._study_id)

# Copy individual trial notes

for src_trial, dst_trial in zip(src_study.get_trials(), dst_study.get_trials()):

note = get_note_from_system_attrs(system_attrs, src_trial._trial_id)["body"]

save_note_with_version(storage, dst_study._study_id, dst_trial._trial_id, 0, note)

# Copy study note

note = get_note_from_system_attrs(system_attrs, None)["body"]

save_note_with_version(storage, dst_study._study_id, None, 0, note)

c-bata · 2023-11-28T03:01:54Z

optuna_dashboard/_app.py

@@ -176,6 +177,7 @@ def delete_study(study_id: int) -> dict[str, Any]:
            delete_all_artifacts(artifact_store, storage, study_id)

        try:
+            note.delete_study_notes(storage, study_id)


study_system_attrs will be removed in storage.delete_study() so that you don't need to delete notes here.

Suggested change

note.delete_study_notes(storage, study_id)

c-bata

Thanks for the update! Looks great to me. I left one minor comment though.

c-bata · 2023-11-29T00:41:23Z

optuna_dashboard/_app.py

@@ -165,6 +165,7 @@ def rename_study(study_id: int) -> dict[str, Any]:
            response.status = 500
            return {"reason": "Failed to load the new study"}

+        note.copy_notes(storage, src_study, dst_study)


Could you move this line to under the L155 to make it show error messages to users?

c-bata

Thanks for the update! LGTM.

adjeiv added 3 commits November 12, 2023 10:49

Retain trial notes

2965e69

Merge branch 'main' of https://github.com/adjeiv/optuna-dashboard int…

4a9457e

…o save_sys_attrs

Tests, formatting

319448a

adjeiv changed the title ~~Save sys attrs~~ Ensure trial notes are transferred when renamed Nov 18, 2023

Typing info

11e38ea

adjeiv marked this pull request as ready for review November 26, 2023 17:32

c-bata requested changes Nov 27, 2023

View reviewed changes

optuna_dashboard/_note.py Outdated Show resolved Hide resolved

c-bata self-assigned this Nov 27, 2023

c-bata reviewed Nov 28, 2023

View reviewed changes

adjeiv added 2 commits November 28, 2023 13:57

Transfer to copy, and remove delete API fns

d52939b

Remove added files

6873b3b

c-bata reviewed Nov 29, 2023

View reviewed changes

Move line so any exceptions are caught

caa2d0c

c-bata approved these changes Nov 30, 2023

View reviewed changes

c-bata merged commit 9eeda40 into optuna:main Nov 30, 2023
14 checks passed

adjeiv deleted the save_sys_attrs branch December 24, 2023 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure trial notes are transferred when renamed #703

Ensure trial notes are transferred when renamed #703

adjeiv commented Nov 18, 2023 •

edited

Loading

codecov bot commented Nov 18, 2023 •

edited

Loading

c-bata left a comment

adjeiv commented Nov 27, 2023 •

edited

Loading

c-bata left a comment

c-bata Nov 28, 2023

c-bata Nov 28, 2023

c-bata left a comment

c-bata Nov 29, 2023

c-bata left a comment

Ensure trial notes are transferred when renamed #703

Ensure trial notes are transferred when renamed #703

Conversation

adjeiv commented Nov 18, 2023 • edited Loading

Contributor License Agreement

Reference Issues/PRs

What does this implement/fix? Explain your changes.

codecov bot commented Nov 18, 2023 • edited Loading

Codecov Report

c-bata left a comment

Choose a reason for hiding this comment

adjeiv commented Nov 27, 2023 • edited Loading

c-bata left a comment

Choose a reason for hiding this comment

c-bata Nov 28, 2023

Choose a reason for hiding this comment

c-bata Nov 28, 2023

Choose a reason for hiding this comment

c-bata left a comment

Choose a reason for hiding this comment

c-bata Nov 29, 2023

Choose a reason for hiding this comment

c-bata left a comment

Choose a reason for hiding this comment

adjeiv commented Nov 18, 2023 •

edited

Loading

codecov bot commented Nov 18, 2023 •

edited

Loading

adjeiv commented Nov 27, 2023 •

edited

Loading