Skip to content

Conversation

@k-collie
Copy link

@k-collie k-collie commented Jan 13, 2025

Description of work

Motivation

  • There's both a setup.py and pyproject.toml at the moment, each specifying different dependency constraints
  • pip uses the setup.py dependencies when installing from pypi, the unconstrained dependency on Flask results in Flask==3.1.0 being installed, which fails the unit tests. It would be nice to add some constraints when we know some dependency versions are incompatible.
  • pip install . uses pyproject.toml dependencies, resulting in different dependency versions when installing from local clone vs from pypi
  • Currently server dependencies are always installed, it would be nice if these were optional for client-only users (especially if we also constrain Flask as this could result in conflicts in their environment).

Changes

  • Consolidated setup.py and pyproject.toml metadata into pyproject.toml
  • Updated requires-python to >=3.9 for new poetry-core version
  • Use newer poetry and standard pyproject.toml format
  • Made server dependencies into an optional group
    • Will require a major version update since server dependencies are removed by default
  • Removed custom VERSION file and used importlib_metadata standard method to expose package version number

Possible further changes

  • Can restrict further to Flask<1 if we know Flask>1 doesn't work
    • Tests all passed and coverage was good when ran with Flask==1.1.4, couldn't see any obvious problems in the few uncovered lines
    • Maybe it's job of dependencies to restrict Flask to <1 if they require?
    • Best to keep dependencies loose unless we know it's broken
  • Could add up to date maintainers list to pyproject.toml
    • Who should be on this list?
  • Removed requirements.txt and requirements_dev.txt
    • poetry.lock should replace requirements_dev.txt sufficiently
    • We could use poetry-export precommit hook to keep a requirements.txt in sync if desired

To test

  • Run client and dataclass tests without server optional group installed
  • Possibly test with concrete SAL server implementation
  • Test client with numpy>=2 installed? No constraint on numpy for pypi installs prior to this MR anyway

Reviewer checklist

  • The author suggested tests are successful.
  • Relevant documentation (user and developer) has been added, and is clear and complete.
  • Suitable unit tests have been added. If unit tests are not included in this MR, please comment as to why and create an issue to address this.

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.

1 participant