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

Limit the maximum number of trials to be added for the quicker first response #719

Merged
merged 11 commits into from
Feb 6, 2024

Conversation

nabenabe0928
Copy link
Collaborator

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

As the first response becomes significantly slower as the number of trials increases, this PR limits the number of trials to be added at the first response.
More specifically, each GET can take only n=limit trials.

What does this implement/fix? Explain your changes.

I added a query parameter for GET of trials.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (522b6ff) 68.96% compared to head (f838ef5) 69.07%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #719      +/-   ##
==========================================
+ Coverage   68.96%   69.07%   +0.10%     
==========================================
  Files          35       35              
  Lines        2388     2393       +5     
==========================================
+ Hits         1647     1653       +6     
+ Misses        741      740       -1     

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

@c-bata c-bata self-assigned this Dec 4, 2023
@nabenabe0928 nabenabe0928 force-pushed the enhance/speedup-get-trials branch 2 times, most recently from f4aafb7 to e50288c Compare December 7, 2023 03:38
@nabenabe0928
Copy link
Collaborator Author

nabenabe0928 commented Dec 7, 2023

@c-bata
This PR will be ready after PR#727 is merged.

However, I think it would be better to separate this PR into smaller PRs as this PR has multiple modifications:

  1. Limiting the maximum number of trials to be used for one update,
  2. Updating studyDetail only if the fetched trials has a positive length to avoid unnecessary renderings,
  3. Detecting the leftovers in the cache and quickly picking them up with a shorter interval, and
  4. Replacing the heuristic for the check interval of the storage with a new algorithm to avoid unnecessary checks.

I will count on you for the decision, but if you believe it is better to split this PR, please tell me which modifications should be bundled.

@nabenabe0928 nabenabe0928 marked this pull request as ready for review December 8, 2023 06:41
@keisuke-umezawa keisuke-umezawa self-assigned this Dec 14, 2023
@nabenabe0928 nabenabe0928 force-pushed the enhance/speedup-get-trials branch 3 times, most recently from a7a21ce to a7edb1c Compare December 18, 2023 11:48
@nabenabe0928
Copy link
Collaborator Author

@c-bata @keisuke-umezawa The PR is ready for reviews as the PR#727 was merged:)

Copy link
Member

@keisuke-umezawa keisuke-umezawa left a comment

Choose a reason for hiding this comment

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

I left some nits and questions. Overall it seems good to me, but how did you assure the quality of code? If possible, you can add some unit test to guarantee it, or at least I would like to check it with some scripts.

optuna_dashboard/ts/action.ts Outdated Show resolved Hide resolved
optuna_dashboard/ts/action.ts Outdated Show resolved Hide resolved
optuna_dashboard/ts/state.ts Outdated Show resolved Hide resolved
optuna_dashboard/_app.py Outdated Show resolved Hide resolved
@nabenabe0928 nabenabe0928 force-pushed the enhance/speedup-get-trials branch 2 times, most recently from 569bfea to 1562c12 Compare December 25, 2023 07:22
@nabenabe0928
Copy link
Collaborator Author

@keisuke-umezawa I added some tests")

Copy link
Member

@keisuke-umezawa keisuke-umezawa left a comment

Choose a reason for hiding this comment

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

Thank you for working on it! LGTM.

@keisuke-umezawa
Copy link
Member

@c-bata
Do you want to review it? If not, I will merge it.

More specifically, this PR makes the following changes:
1. Add query param for limit,
2. Update studyDetails only if there is no study with
   the specified study_id or the fetched trials has a
   positive length,
3. Shorten the waiting interval when there is a leftover
   in the server side, and
4. Adapt the flake8 setup to the Optuna repo.
@nabenabe0928
Copy link
Collaborator Author

import optuna

def objective_many_trials_imbalance_importances(trial: optuna.Trial) -> float:
    if (trial.number + 1) % 500 == 0:
        print(f"Evaluate Trial#{trial.number}")

    x1 = trial.suggest_float("x1", 0, 10)
    x2 = trial.suggest_float("x2", 0, 10)
    x3 = trial.suggest_categorical("x3", ["foo", "bar"])
    return (x1 - 2) ** 2 + (x2 - 5) ** 2

storage = optuna.storages.InMemoryStorage()
study = optuna.create_study(study_name="test", storage=storage, sampler=optuna.samplers.RandomSampler())
study.optimize(objective_single, n_trials=10000)

Screenshot 2024-01-16 17 46 24

@keisuke-umezawa keisuke-umezawa merged commit cc26f0e into optuna:main Feb 6, 2024
17 of 18 checks passed
c-bata added a commit to c-bata/optuna-dashboard that referenced this pull request Feb 9, 2024
…up-get-trials"

This reverts commit cc26f0e, reversing
changes made to 83b548f.
This was referenced Feb 9, 2024
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