Refactor: Change the DeclarativeBase sqlalchemy style

[HeloiseJoffe]

closed: #612

Changes:

• Replaced declarative_base with DeclarativeBase style
• Cast 'table' to Table type to satisfy mypy

Questions:
There is other new styles in SQLAlchemy, for example the declarative use of Column is replaced by mapped_column.
Should this be applied as well?
In the Bookkeeping schema, this has been done, but the other changes haven’t been applied. Is there a reason for that?

[ryuwd]

All the steps under https://docs.sqlalchemy.org/en/20/changelog/whatsnew_20.html#migrating-an-existing-mapping should be followed before closing #612

substituting mapped_column is enough to start fixing the typing errors but we lose the usefulness of the type checks without the additional annotations (which I still need to do for the bookkeeping schema)

[ryuwd]

Thanks for working on this! Next steps:

1. Column to mapped_column (see diff comment example in job_logging/schema.py)
2. Add Mapped[...] annotations for all schema files
3. Remove cast(__table__, Table) everywhere

[read-the-docs-community]

Documentation build overview

📚 diracx | 🛠️ Build #31365623 | 📁 Comparing a7e2ada against latest (60e5846)

🔍 Preview build

Show files changed (2 files in total): 📝 2 modified | ➕ 0 added | ➖ 0 deleted

File	Status
dev/reference/coding-conventions/index.html	📝 modified
dev/explanations/components/db/index.html	📝 modified

[ryuwd]

This is looking great! I only have small questions remaining

[ryuwd]

Can all of these be removed?

[HeloiseJoffe]

I kept them for compatibility reasons, in case other extensions or modules use them. Once it's confirmed that no other code uses them, they can be removed.

[ryuwd]

Would you be happy to check?

[HeloiseJoffe]

These helpers aren’t used by diracx itself, but I can’t tell whether external or private extensions rely on them. Should I remove them anyway?

[ryuwd]

Yes, please remove them. Extensions should update to be consistent with us.

[aldbr]

Theoretically we should avoid to remove them in one go because there are explicitly part of the public API that extensions can use (see https://github.com/DIRACGrid/diracx/pull/748/changes#diff-f2cfd457649455e1dbe7583fcd08cbd9524ed972d423b420d10a526b5083dd7eL6-L8). Extensions already relying on it could then be broken.
It's a breaking change, so ideally, it should be first deprecated and then removed in a major release.

In practice, there are not a lot of extensions in production (only LHCb? May be CTAO?).
As a good practice, you can first add a deprecation warning if one tries to use one of these types, open an issue to explain that you want to remove them. Then we can briefly discuss about that during a Dops meeting, just to make sure no one is using them. And then make another PR based on the issue you opened to remove them.

[aldbr]

(The process is heavy, and this is why we should be very careful in what we want to make public through the __all__ keywords)

[ryuwd]

Thank you for the hard work! I'm happy with how you've migrated the tables, there are just a couple more old utility functions we should try to get rid of if possible (see comments)

A TODO for the future - we should keep track with tests and test fixtures how these schema definitions change the DB schema expected by SQLAlchemy

[HeloiseJoffe]

I checked the schema changes using a script that extracts the DDL from the SQLAlchemy metadata.
I generated the DDL on the main branch and on this PR branch, then compared the two outputs.
There was no structural difference.

[ryuwd]

LGTM !

[aldbr]

I just have one last minor comment. The issue was initially opened for that specific comment: #608 (comment)

Basically to change Any in search and summary I guess:

diracx/diracx-db/src/diracx/db/sql/utils/base.py

Line 253 in faf6b44

table: Any,

diracx/diracx-db/src/diracx/db/sql/utils/base.py

Line 293 in faf6b44

self, table: Any, group_by: list[str], search: list[SearchSpec]

Do you think we can have something better now?

[aldbr]

Very last comment, sorry 😅
While I was going through the open issues, I noticed that #433 was very similar but contains a few additional info about type ignore and mypy.
Could you check that please?