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.

Sign up for GitHub

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

Jump to bottom

Move default logic of get_trial_id_from_study_id_trial_number() method to BaseStorage. #3910

Merged
merged 4 commits into from
Sep 13, 2022
Merged

Move default logic of get_trial_id_from_study_id_trial_number() method to BaseStorage. #3910

merged 4 commits into from
Sep 13, 2022

Conversation

c-bata
Copy link
Member

@c-bata c-bata commented Aug 22, 2022

Motivation

Follow-up #3909. Please see the description of #3909.

Description of the changes

Implement default logic of get_trial_id_from_study_id_trial_number in BaseStorage.

@c-bata c-bata changed the title Refactor get trial id from study id trial number Move default logic of get_trial_id_from_study_id_trial_number() method to BaseStorage. Aug 22, 2022
@github-actions github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label Aug 22, 2022
@github-actions
Copy link
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Aug 29, 2022
@c-bata c-bata force-pushed the refactor-get-trial-id-from-study-id-trial-number branch from 584b18b to ce674ac Compare August 30, 2022 09:50
@c-bata c-bata marked this pull request as ready for review August 30, 2022 10:35
@c-bata c-bata removed the stale Exempt from stale bot labeling. label Aug 30, 2022
@c-bata c-bata assigned hvy and knshnb Aug 30, 2022
@c-bata
Copy link
Member Author

c-bata commented Aug 30, 2022

@hvy @knshnb Could you review this PR if you have time?

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022 •
edited

Codecov Report

Merging #3910 (86f7081) into master (2c09c5d) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3910      +/-   ##
==========================================
- Coverage   90.58%   90.55%   -0.03%     
==========================================
  Files         167      167              
  Lines       13113    13110       -3     
==========================================
- Hits        11878    11872       -6     
- Misses       1235     1238       +3
Impacted Files Coverage Δ
optuna/storages/_base.py 79.46% <0.00%> (-2.19%) ⬇️
optuna/study/_tell.py 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@c-bata c-bata unassigned hvy Sep 2, 2022
@c-bata
Copy link
Member Author

c-bata commented Sep 2, 2022

@wattlebirdaz Could you review this PR since hvy-san will take a vacation.

rotaki
rotaki approved these changes Sep 2, 2022
View reviewed changes
Copy link
Collaborator

@rotaki rotaki left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. LGTM.

knshnb
knshnb reviewed Sep 5, 2022
View reviewed changes
Copy link
Member

@knshnb knshnb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think this PR is generally a good direction, but I left some concerns and questions. PTAL.

tests/study_tests/test_study.py Outdated
Comment on lines 1485 to 1493
def test_tell_storage_not_implemented_trial_number() -> None:
with StorageSupplier("inmemory") as storage:

with patch.object(
storage,
"get_trial_id_from_study_id_trial_number",
side_effect=NotImplementedError,
):
study = create_study(storage=storage)

study.tell(study.ask(), 1.0)

# Storage missing implementation for method required to map trial numbers back to
# trial IDs.
with pytest.warns(UserWarning):
study.tell(study.ask().number, 1.0)

with pytest.raises(ValueError):
study.tell(study.ask().number + 1, 1.0)
study = create_study(storage=storage)
study.tell(study.ask(), 1.0)
study.tell(study.ask().number, 1.0)

with pytest.raises(ValueError):
study.tell(study.ask().number + 1, 1.0)
Copy link
Member

Choose a reason for hiding this comment

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

With this, InMemoryStorage.get_trial_id_from_study_id_trial_number is tested instead of BaseStorage.get_trial_id_from_study_id_trial_number.

Copy link
Member

Choose a reason for hiding this comment

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

I cannot think of a way to easily test InMemoryStorage.get_trial_id_from_study_id_trial_number (except for mocking all the abstract methods of InMemoryStorage), but do you come up with a good way...?

Copy link
Member Author

@c-bata c-bata Sep 13, 2022 •
edited

Choose a reason for hiding this comment

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

Good catch! It is a difficult problem how to test concrete methods in an abstract base class. I thought it would be better to just remove this test case since this test case should not be written in test_study.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that get_trial_id_from_study_id_trial_number() is enough tested at:

optuna/tests/storages_tests/test_storages.py

Lines 1070 to 1096 in 2c09c5d

@pytest.mark.parametrize("storage_mode", STORAGE_MODES)
def test_get_trial_id_from_study_id_trial_number(storage_mode: str) -> None:
with StorageSupplier(storage_mode) as storage:
with pytest.raises(KeyError): # Matching study does not exist.
storage.get_trial_id_from_study_id_trial_number(study_id=0, trial_number=0)
study_id = storage.create_new_study()
with pytest.raises(KeyError): # Matching trial does not exist.
storage.get_trial_id_from_study_id_trial_number(study_id, trial_number=0)
trial_id = storage.create_new_trial(study_id)
assert trial_id == storage.get_trial_id_from_study_id_trial_number(
study_id, trial_number=0
)
# Trial IDs are globally unique within a storage but numbers are only unique within a
# study. Create a second study within the same storage.
study_id = storage.create_new_study()
trial_id = storage.create_new_trial(study_id)
assert trial_id == storage.get_trial_id_from_study_id_trial_number(
study_id, trial_number=0
)

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this test should not be located in test_study.py.
Still, I feel the default logic of BaseStorage.get_trial_id_from_study_id_trial_number should be tested somewhere (not tested in test_get_trial_id_from_study_id_trial_number because all the concrete class override the method). We should probably discuss how to test default implementations in BaseStorage as another issue.

Copy link
Member

Choose a reason for hiding this comment

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

So, let me merge this PR first. If you have any opinions, please leave a comment!

optuna/study/_tell.py Show resolved Hide resolved
@github-actions
Copy link
Contributor

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Sep 12, 2022
@c-bata c-bata added enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. and removed stale Exempt from stale bot labeling. labels Sep 13, 2022
@knshnb knshnb added this to the v3.1.0 milestone Sep 13, 2022
knshnb
knshnb approved these changes Sep 13, 2022
View reviewed changes
Copy link
Member

@knshnb knshnb left a comment

Choose a reason for hiding this comment

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

Thanks for the update! LGTM. Please comment if you have any opinion on the point I mentioned.

@knshnb knshnb merged commit de29496 into optuna:master Sep 13, 2022
@c-bata c-bata deleted the refactor-get-trial-id-from-study-id-trial-number branch September 13, 2022 13:49
@c-bata
Copy link
Member Author

c-bata commented Sep 13, 2022

Thank you for your thorough review!
In general it is difficult to test concrete methods in an abstract base class. I think the fundamental solution is to avoid implementing concrete methods in RDBStorage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knshnb knshnb knshnb approved these changes

@rotaki rotaki rotaki approved these changes

Assignees

@knshnb knshnb

Labels
enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions.
Projects
None yet
Milestone
v3.1.0-b0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@c-bata @codecov-commenter @knshnb @rotaki @hvy