Skip to content

Conversation

@gfrn
Copy link
Collaborator

@gfrn gfrn commented Jan 29, 2025

  • Bump database schema version

  • Drop old Python versions

  • Update DB tarfile artifact reference

  • Update Python version requirements in README

  • Pin sqlacodegen version to 3.0.0rc5

  • Update sqlacodegen command

Tests failed for one of the "generate ORM PR" actions, which I imagine is because it's duplicated (once for the push, and once for the pull request), part of having merged this from a forked repo

I am also not too sure of how well this might work with 1.4. SQLAlchemy 2.0 introduces a ton of breaking changes, and sqlacodegen 2 was making this way more painful than it had to be. Primarily because it has no support for SQLAlchemy 2.0 and all the new features it has.

3.0.0rc2 is the last (only?) version of sqlacodegen to "support" both 1.4 and 2.0, and even then, it fails if you follow their docs.

2.0.0 never really supported SQLAlchemy 2.0 (https://github.com/agronholm/sqlacodegen/blob/60392056b95a0665d4af67a8403eb998a92a5e7e/pyproject.toml)

* Bump database schema version

* Drop old Python versions

* Update DB tarfile artifact reference

* Update Python version requirements in README

* Pin sqlacodegen version to 3.0.0rc5

* Update sqlacodegen command
@codecov
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 92.19%. Comparing base (42f5a57) to head (9f0bb18).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #227      +/-   ##
==========================================
+ Coverage   91.99%   92.19%   +0.20%     
==========================================
  Files          26       27       +1     
  Lines        3735     4140     +405     
  Branches      252      166      -86     
==========================================
+ Hits         3436     3817     +381     
- Misses        206      230      +24     
  Partials       93       93              
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gfrn gfrn requested a review from ndevenish January 29, 2025 15:06
@gfrn gfrn marked this pull request as draft February 7, 2025 13:28
@gfrn gfrn marked this pull request as ready for review February 7, 2025 15:11
Copy link
Collaborator

@ndevenish ndevenish left a comment

Choose a reason for hiding this comment

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

We should probably completely drop the python versions e.g. also change in https://github.com/DiamondLightSource/ispyb-api/pull/227/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R25. Otherwise looks okay, as long as it's doing what you need?

@ndevenish
Copy link
Collaborator

I'm left a little confused how this leaves the ispyb-api support for sqlalchemy<2. I don't really have any objection to dropping 1.4, as long as we don't have anything critical depending on it (or can get away with not having the latest schema while we update them).

I'm probably not against just dropping 1.4.

Co-authored-by: Nicholas Devenish <ndevenish@gmail.com>
@gfrn
Copy link
Collaborator Author

gfrn commented Feb 18, 2025

I'm left a little confused how this leaves the ispyb-api support for sqlalchemy<2. I don't really have any objection to dropping 1.4, as long as we don't have anything critical depending on it (or can get away with not having the latest schema while we update them).

I'm probably not against just dropping 1.4.

I've tried it out with 1.4, and although you can kludge it into working, I wouldn't say it's supported. sqlacodegen itself doesn't support 1.4 anymore, and the generated code makes use of typed column declarations, which is the recommended way to go with 2. 1.4 doesn't support that without intermediaries.

I would say that this doesn't support 1.4, isn't intended to work with 1.4, and we can't reasonably provide two separate schemas, unfortunately. This is a breaking change, so I suggest we bump it up by a major version, and possibly release it as beta for the moment being

@ndevenish
Copy link
Collaborator

@gfrn

        # Get child AutoProcScalingStatistics entries
>       stats = aps.AutoProcScalingStatistics
E       AttributeError: 'AutoProcScaling' object has no attribute 'AutoProcScalingStatistics'

tests/sqlalchemy/test_models.py:48: AttributeError

It looks like you've changed the way backreferences are generated here (Absolutely not implying this was bad by asking, but what was the reason for this?), does this error make sense to you in relation to that?

ndevenish and others added 7 commits March 11, 2025 15:22
Generated with
sqlacodegen            3.0.0rc5
SQLAlchemy             2.0.38

Co-authored-by: Guilherme Francisco <guifreitas89@hotmail.com>
Co-authored-by: ISPyB-API Azure build <DiamondLightSource-build-server@users.noreply.github.com>
At least update_orm fails if on a PR branch, this should avoid that
and also avoid running twice (pushed to a PR branch).
@ndevenish ndevenish self-requested a review March 11, 2025 16:24
@ndevenish ndevenish merged commit 1a2b201 into main Mar 11, 2025
12 checks passed
@ndevenish ndevenish deleted the bump-schema-version branch March 11, 2025 16:34
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