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

Remove version pins for SQLA & Alembic #247

Closed
wants to merge 1 commit into from

Conversation

gordthompson
Copy link
Collaborator

No description provided.

@gordthompson gordthompson requested a review from rafiss July 9, 2024 22:03
Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

the way this used to work is the unpinned versions are specified in

# Packages used when running tests. Installed by tox into multiple
# virtualenvs.
#
# To add/update dependencies, update test-requirements.in (not the
# generated test-requirements.txt) and run make update-requirements

and make update-requirements is used to update dependencies but keep them pinned here in order to keep the build stable. (e.g., we used to have issues where CI could fail without us making any changes in this repo, if a CI run pulled in a newer version of sqlalchemy.) does this workflow no longer work?

@gordthompson
Copy link
Collaborator Author

does this workflow no longer work?

It's awkward. While CI is passing here I can no longer run make update-requirements locally because greenlet is pinned at 2.0.2 and tox can't build it on my machine. If I unpin it then 3.0.3 gets installed and that works. I haven't looked into it too closely but it may be because my .venv has Python 3.12.

Also, pinned test-requirements means that we're not testing against the latest versions of SQLAlchemy and Alembic (and dependencies) unless we continually make update-requirements. If I do that then I'd have to submit a PR and bother you each time (like this time) because the master branch is protected so I can't approve my own changes, even if all tests pass.

@rafiss
Copy link
Contributor

rafiss commented Jul 11, 2024

Also, pinned test-requirements means that we're not testing against the latest versions of SQLAlchemy and Alembic (and dependencies) unless we continually make update-requirements.

I actually view this as a feature, not a bug. It is desirable to know that re-running the same CI execution is guaranteed to achieve the same results and use the same dependencies. To give a more extreme but analogous example, we would also not want to un-pin the version of python used in CI, since that could cause instability across different test runs. It's better to be explicit about when we change the version of anything (be it python itself or a dependency), that way we can decide when we need to deal with potentially breaking changes instead of that task being forced upon us.

You're right, there certainly is a trade-off here between stability versus toil, but I would like to err on the side of stability.

I created #248 after running make update-requirements myself. This removed the greenlet dependency. Does that resolve the original issue you had?

@gordthompson gordthompson mentioned this pull request Jul 11, 2024
@gordthompson
Copy link
Collaborator Author

superseded by #248

@gordthompson gordthompson deleted the gordthompson-patch-1 branch July 18, 2024 12:18
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.

2 participants