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

c-bata · 2023-01-05T08:04:45Z

Motivation

CuPy team reported that Optuna depends on some SQLAlchemy's features that will be removed in the SQLAlchemy 2.0 release.
cupy/cupy#7276

Description of the changes

Fix warnings of sqlalchemy.exc.RemovedIn20Warning. We can detect them like:

$ SQLALCHEMY_WARN_20=1 pytest tests/storages_tests -W error::sqlalchemy.exc.RemovedIn20Warning

c-bata · 2023-01-05T08:15:47Z

Let me set this PR's milestone v3.1.0 since SQLAlchemy v2.0 will be released in January.

Version 2.0.0rc1 is hoped to be very close to the final 2.0.0 release which is anticipated in January. A few additional changes have been added since the 2.0.0b4 release, mostly in the area of Session behavior.
https://www.sqlalchemy.org/blog/#sqlalchemy-2.0.0rc1-released

codecov-commenter · 2023-01-05T08:45:23Z

Codecov Report

Merging #4304 (65abd8a) into master (f2945b4) will decrease coverage by 0.23%.
The diff coverage is 57.57%.

@@            Coverage Diff             @@
##           master    #4304      +/-   ##
==========================================
- Coverage   89.77%   89.53%   -0.24%     
==========================================
  Files         170      170              
  Lines       13265    13389     +124     
==========================================
+ Hits        11908    11988      +80     
- Misses       1357     1401      +44

Impacted Files	Coverage Δ
optuna/storages/_rdb/alembic/versions/v1.3.0.a.py	`65.51% <50.00%> (-1.76%)`	⬇️
optuna/storages/_rdb/alembic/versions/v2.4.0.a.py	`90.52% <50.00%> (-1.87%)`	⬇️
optuna/storages/_rdb/alembic/versions/v3.0.0.a.py	`68.06% <50.00%> (-0.90%)`	⬇️
optuna/storages/_rdb/alembic/versions/v3.0.0.b.py	`83.33% <50.00%> (-2.95%)`	⬇️
optuna/storages/_rdb/alembic/versions/v3.0.0.c.py	`53.26% <50.00%> (-0.68%)`	⬇️
optuna/storages/_rdb/alembic/versions/v3.0.0.d.py	`51.00% <50.00%> (-0.55%)`	⬇️
optuna/storages/_rdb/models.py	`97.68% <50.00%> (-0.65%)`	⬇️
optuna/storages/_rdb/storage.py	`93.90% <100.00%> (+0.02%)`	⬆️
optuna/trial/_fixed.py	`89.69% <0.00%> (-5.19%)`	⬇️
optuna/trial/_frozen.py	`94.76% <0.00%> (-2.63%)`	⬇️
... and 4 more

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

c-bata · 2023-01-05T09:17:58Z

optuna/storages/_rdb/models.py

+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


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()

c-bata · 2023-01-05T09:18:26Z

optuna/storages/_rdb/storage.py

+        connection = self.engine.connect()
+        context = alembic_migration.MigrationContext.configure(connection)
+        with connection.begin():
+            script = self._create_alembic_script()
+            context.stamp(script, revision)


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)

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

optuna/tests/storages_tests/rdb_tests/test_storage.py

Lines 139 to 142 in 4d06898

# 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.

Good catch!

c-bata · 2023-01-10T03:51:21Z

FYI: I pushed a temporary change to run tests with sqlalchemy 2.0.0rc2 at e57e151.

contramundum53

LGTM!

c-bata · 2023-01-10T04:31:31Z

Thank you for your swift review. I'll revert e57e151 after reviewed by yanase-san 🙇

toshihikoyanase

By using optuna-e2e, I confirmed that the change in the migration scripts worked with SQLAlchemy==[1.3.0, 1.4.46, 2.0.0rc2]. (Actually mssql did not worked with SQLAlchemy==1.3.0, but I don't think it is related to this PR.) LGTM!

@c-bata could you revert the tentative change in the CI settings?

toshihikoyanase · 2023-01-11T09:43:40Z

optuna/storages/_rdb/storage.py

+        connection = self.engine.connect()
+        context = alembic_migration.MigrationContext.configure(connection)
+        with connection.begin():
+            script = self._create_alembic_script()
+            context.stamp(script, revision)


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

optuna/tests/storages_tests/rdb_tests/test_storage.py

Lines 139 to 142 in 4d06898

# 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.

This reverts commit e57e151.

c-bata · 2023-01-11T10:09:30Z

@toshihikoyanase Thank you for your review! I've just pushed 3d20af1 to revert the tentative change.

toshihikoyanase · 2023-01-11T11:20:30Z

I confirmed that the CI settings were reverted. Let me merge this PR.

c-bata added 2 commits January 5, 2023 16:52

Avoid to use sqlalchemy.ext.declarative

b1df10c

Detect sqlalchemy.exc.RemovedIn20Warning on GitHub Actions

cbafba2

github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label Jan 5, 2023

Call .begin() explicitly

65abd8a

c-bata added this to the v3.1.0 milestone Jan 5, 2023

c-bata marked this pull request as ready for review January 5, 2023 08:33

c-bata commented Jan 5, 2023

View reviewed changes

toshihikoyanase self-assigned this Jan 6, 2023

toshihikoyanase added the code-fix Change that does not change the behavior, such as code refactoring. label Jan 6, 2023

HideakiImamura assigned contramundum53 Jan 6, 2023

c-bata added 2 commits January 10, 2023 12:50

Add temporary changes to run tests with sqlalchemy 2.0rc2

e57e151

Merge branch 'master' into remove-sqlalchemy-20-warnings

9b16291

Remove warning check for sqlalchemy.exc.RemovedIn20Warning

ae5bf5a

contramundum53 approved these changes Jan 10, 2023

View reviewed changes

c-bata unassigned contramundum53 Jan 11, 2023

toshihikoyanase approved these changes Jan 11, 2023

View reviewed changes

Revert "Add temporary changes to run tests with sqlalchemy 2.0rc2"

3d20af1

This reverts commit e57e151.

toshihikoyanase merged commit 633b7d1 into optuna:master Jan 11, 2023

toshihikoyanase mentioned this pull request Jan 11, 2023

Activate a test case for _set_alembic_revision #4319

Merged

c-bata deleted the remove-sqlalchemy-20-warnings branch January 11, 2023 12:16

24hours mentioned this pull request Jan 30, 2023

[Bug] optuna depending on deprecated feature in sqlalchemy. facebookresearch/hydra#2562

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

c-bata commented Jan 5, 2023 •

edited

c-bata commented Jan 5, 2023

codecov-commenter commented Jan 5, 2023

c-bata Jan 5, 2023 •

edited

c-bata Jan 5, 2023 •

edited

toshihikoyanase Jan 11, 2023 •

edited

c-bata Jan 11, 2023

c-bata commented Jan 10, 2023

contramundum53 left a comment

c-bata commented Jan 10, 2023

toshihikoyanase left a comment •

edited

toshihikoyanase Jan 11, 2023 •

edited

c-bata commented Jan 11, 2023

toshihikoyanase commented Jan 11, 2023

	# 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()

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

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

Conversation

c-bata commented Jan 5, 2023 • edited

Motivation

Description of the changes

c-bata commented Jan 5, 2023

codecov-commenter commented Jan 5, 2023

Codecov Report

c-bata Jan 5, 2023 • edited

Choose a reason for hiding this comment

c-bata Jan 5, 2023 • edited

Choose a reason for hiding this comment

toshihikoyanase Jan 11, 2023 • edited

Choose a reason for hiding this comment

c-bata Jan 11, 2023

Choose a reason for hiding this comment

c-bata commented Jan 10, 2023

contramundum53 left a comment

Choose a reason for hiding this comment

c-bata commented Jan 10, 2023

toshihikoyanase left a comment • edited

Choose a reason for hiding this comment

toshihikoyanase Jan 11, 2023 • edited

Choose a reason for hiding this comment

c-bata commented Jan 11, 2023

toshihikoyanase commented Jan 11, 2023

c-bata commented Jan 5, 2023 •

edited

c-bata Jan 5, 2023 •

edited

c-bata Jan 5, 2023 •

edited

toshihikoyanase Jan 11, 2023 •

edited

toshihikoyanase left a comment •

edited

toshihikoyanase Jan 11, 2023 •

edited