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

Avoid to use features that will be removed in SQLAlchemy v2.0 #4304

Merged
merged 7 commits into from
Jan 11, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/tests-storage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ jobs:
strategy:
matrix:
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11']
sqlalchemy-version: ['1.4.46', '2.0.0rc2']

services:
mysql:
Expand Down Expand Up @@ -93,6 +94,9 @@ jobs:

pip install --progress-bar off .[test]
pip install --progress-bar off .[optional]
pip install --progress-bar off sqlalchemy==$SQLALCHEMY_VERSION
env:
SQLALCHEMY_VERSION: ${{ matrix.sqlalchemy-version }}

- name: Install DB bindings
run: |
Expand All @@ -102,6 +106,7 @@ jobs:
run: |
pytest tests/storages_tests/test_with_server.py
env:
SQLALCHEMY_WARN_20: 1
OMP_NUM_THREADS: 1
TEST_DB_URL: mysql+pymysql://user:test@127.0.0.1/optunatest

Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ jobs:
strategy:
matrix:
python-version: ['3.7', '3.8', '3.9', '3.10', '3.11']
sqlalchemy-version: ['1.4.46', '2.0.0rc2']

services:
redis:
Expand Down Expand Up @@ -65,13 +66,20 @@ jobs:

pip install --progress-bar off .[test]
pip install --progress-bar off .[optional]
pip install --progress-bar off sqlalchemy==$SQLALCHEMY_VERSION
env:
SQLALCHEMY_VERSION: ${{ matrix.sqlalchemy-version }}

- name: Scheduled tests
if: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' }}
run: |
pytest tests -m "not integration"
env:
SQLALCHEMY_WARN_20: 1

- name: Tests
if: ${{ github.event_name != 'schedule' }}
run: |
pytest tests -m "not integration and not slow"
env:
SQLALCHEMY_WARN_20: 1
7 changes: 6 additions & 1 deletion optuna/storages/_rdb/alembic/versions/v1.3.0.a.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@
import sqlalchemy as sa

from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import orm

try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base

# revision identifiers, used by Alembic.
revision = "v1.3.0.a"
down_revision = "v1.2.0.a"
Expand Down
7 changes: 6 additions & 1 deletion optuna/storages/_rdb/alembic/versions/v2.4.0.a.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,16 @@
from sqlalchemy import Integer
from sqlalchemy import UniqueConstraint
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import orm

from optuna.study import StudyDirection

try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base


# revision identifiers, used by Alembic.
revision = "v2.4.0.a"
Expand Down
7 changes: 6 additions & 1 deletion optuna/storages/_rdb/alembic/versions/v3.0.0.a.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from sqlalchemy import Text
from sqlalchemy import UniqueConstraint
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.declarative import declarative_base

from optuna.distributions import _convert_old_distribution_to_new_distribution
from optuna.distributions import BaseDistribution
Expand All @@ -36,6 +35,12 @@
from optuna.distributions import UniformDistribution
from optuna.trial import TrialState

try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base


# revision identifiers, used by Alembic.
revision = "v3.0.0.a"
Expand Down
7 changes: 6 additions & 1 deletion optuna/storages/_rdb/alembic/versions/v3.0.0.b.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@
from sqlalchemy import Float
from sqlalchemy import ForeignKey
from sqlalchemy import Integer
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import Session

try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base


# revision identifiers, used by Alembic.
revision = "v3.0.0.b"
Expand Down
7 changes: 6 additions & 1 deletion optuna/storages/_rdb/alembic/versions/v3.0.0.c.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@
from alembic import op
import sqlalchemy as sa
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import orm
from typing import Optional
from typing import Tuple

try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base


# revision identifiers, used by Alembic.
revision = "v3.0.0.c"
Expand Down
7 changes: 6 additions & 1 deletion optuna/storages/_rdb/alembic/versions/v3.0.0.d.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@
from alembic import op
import sqlalchemy as sa
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import orm
from typing import Optional
from typing import Tuple

try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base


# revision identifiers, used by Alembic.
revision = "v3.0.0.d"
Expand Down
8 changes: 7 additions & 1 deletion optuna/storages/_rdb/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,19 @@
from sqlalchemy import String
from sqlalchemy import Text
from sqlalchemy import UniqueConstraint
from sqlalchemy.ext.declarative import declarative_base

from optuna import distributions
from optuna.study._study_direction import StudyDirection
from optuna.trial import TrialState


try:
from sqlalchemy.orm import declarative_base
except ImportError:
# TODO(c-bata): Remove this after dropping support for SQLAlchemy v1.3 or prior.
from sqlalchemy.ext.declarative import declarative_base
Comment on lines +28 to +32
Copy link
Member Author

@c-bata c-bata Jan 5, 2023

Choose a reason for hiding this comment

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

optuna/storages/_rdb/models.py:40
/Users/c-bata/go/src/github.com/optuna/optuna/optuna/storages/_rdb/models.py:40: MovedIn20Warning: The declarative_base() function is now available as sqlalchemy.orm.declarative_base(). (deprecated since: 1.4) (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
BaseModel: Any = declarative_base()



# Don't modify this version number anymore.
# The schema management functionality has been moved to alembic.
SCHEMA_VERSION = 12
Expand Down
8 changes: 5 additions & 3 deletions optuna/storages/_rdb/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -1098,9 +1098,11 @@ def _init_alembic(self) -> None:

def _set_alembic_revision(self, revision: str) -> None:

context = alembic_migration.MigrationContext.configure(self.engine.connect())
script = self._create_alembic_script()
context.stamp(script, revision)
connection = self.engine.connect()
context = alembic_migration.MigrationContext.configure(connection)
with connection.begin():
script = self._create_alembic_script()
context.stamp(script, revision)
Comment on lines +1101 to +1105
Copy link
Member Author

@c-bata c-bata Jan 5, 2023

Choose a reason for hiding this comment

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

tests/storages_tests/rdb_tests/test_storage.py: 11 warnings
/Users/c-bata/go/src/github.com/optuna/optuna/optuna/storages/_rdb/storage.py:1103: RemovedIn20Warning: The current statement is being autocommitted using implicit autocommit, which will be removed in SQLAlchemy 2.0. Use the .begin() method of Engine or Connection in order to use an explicit transaction for DML and DDL statements. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
context.stamp(script, revision)

Copy link
Member

Choose a reason for hiding this comment

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

[Note] I confirmed that this logic worked with the following test case locally.

# TODO(ohta): Remove the following comment out when the second revision is introduced.
# with pytest.raises(RuntimeError):
# storage._set_alembic_revision(storage._version_manager._get_base_version())
# storage._check_table_schema_compatibility()

Maybe we can activate it in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!


def check_table_schema_compatibility(self) -> None:

Expand Down