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

O(1) cached RDB storage trial select queries. #1426

Closed
wants to merge 5 commits into from

Conversation

hvy
Copy link
Member

@hvy hvy commented Jun 24, 2020

Motivation

Follow-up of #1405 (thus depends on it) and addresses #1383. In short, optimizes trial select queries from O(cached trials) to O(1) for the cached RDB storage. The difference becomes clear when the number of trials increase.

Description of the changes

Changes to use the update timestamp when querying trials instead of using an exclusion list that grows linearly as the number of trials increase.

TODO

  • Verify logic.
  • Fix broken enqueue trial tests with SQLite.

Benchmarks (Preliminary. See TODO above)

import time

import optuna
from optuna import logging
from optuna.samplers import TPESampler
import sqlalchemy


def objective(trial, n_params):
    return sum(trial.suggest_float(f"x{i}", 0.0, 1.0) for i in range(n_params))


if __name__ == "__main__":
    n_params = 10
    n_trials = 1000

    storage = "mysql://root@localhost/lastupdate"
    database = "lastupdate"
    engine = sqlalchemy.create_engine(storage)
    conn = engine.connect()
    conn.execute("commit")
    try:
        conn.execute("drop database {}".format(database))
    except:
        pass
    conn.execute("create database {}".format(database))
    conn.close()

    study = optuna.create_study(sampler=TPESampler(), study_name=database, storage=storage, load_if_exists=True)

    start = time.time()
    study.optimize(lambda trial: objective(trial, n_params), n_trials=n_trials)
    end = time.time()
    print('Time', end - start)

PR
Time 43.876389265060425

master
Time 59.81723189353943

You can clearly notice that each trial becomes more time consuming as the number of trials increase. This issue is mitigated by this change.

Note

I decided to not introduce the "last update" attribute to the FrozenTrial as it's not necessary. This is purely an internal performance improvement. It could make debugging in the future difficult though. Open for feedback.

@hvy hvy added the enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. label Jun 24, 2020
@hvy hvy force-pushed the rdb-trial-last-update-incremental-load branch 2 times, most recently from 93b82db to 9c5dbea Compare June 24, 2020 16:13
@hvy hvy force-pushed the rdb-trial-last-update-incremental-load branch from 9c5dbea to 9157174 Compare June 24, 2020 16:22
@hvy
Copy link
Member Author

hvy commented Jun 25, 2020

Lacking precision in datetime_last_update (last column) with sa.func.now().

sqlite> select * from trials;
1|0|1|COMPLETE|50.0|2020-06-25 09:59:33.912881|2020-06-25 09:59:33.988814|2020-06-25 00:59:33
2|1|1|COMPLETE|1.0|2020-06-25 09:59:33.924300|2020-06-25 09:59:34.054696|2020-06-25 00:59:34

For fractional seconds, use the
dialect-specific datatype, such as :class:.mysql.TIME. For
timezone support, use at least the :class:_types.TIMESTAMP datatype,
if not the dialect-specific datatype object.

https://github.com/sqlalchemy/sqlalchemy/blob/660a340bff8fcefd2826032e75210c0924a2335e/lib/sqlalchemy/sql/sqltypes.py#L826

@hvy
Copy link
Member Author

hvy commented Jun 25, 2020

Enqueue tests seem to be failing because the update timestamp is not updated when the TrialParamModel (backref TrialModel) is updated.

@hvy
Copy link
Member Author

hvy commented Jun 25, 2020

In my understanding

  • we need to introduce DateTime columns to related tables (params, {user,syste} attributes) as well, or create a new table. There may be a better options using triggers.
  • Precision becomes an issue since we have to relax the query from > to >= leading to unnecessary data being reloaded.
  • SQLite DateTime with >= doesn't work as expected we need some datetime.timedelta offset, again leading to the same issue described above.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2020

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Jul 9, 2020
@hvy
Copy link
Member Author

hvy commented Jul 28, 2020

Closing this issue for now, for reasons mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Change that does not break compatibility and not affect public interfaces, but improves performance. stale Exempt from stale bot labeling.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant