Skip to content

Make server dependencies non-optional#123

Merged
rwb27 merged 3 commits intomainfrom
non-optional-server-dependencies
Jul 2, 2025
Merged

Make server dependencies non-optional#123
rwb27 merged 3 commits intomainfrom
non-optional-server-dependencies

Conversation

@rwb27
Copy link
Copy Markdown
Collaborator

@rwb27 rwb27 commented Jun 28, 2025

Currently, a labthings client can be installed with pip install labthings_fastapi and the server is installed with pip install labthings_fastapi[server]. #121 makes it possible to import Thing from the top level namespace, which will fail if we don't have the optional server dependencies.

This PR makes the server dependencies non-optional. Doing so will fix some confusion, and stop client-only installs from being broken now that Thing can be imported at top level.

In the future, we should have a separate PyPI package for the client code. I think this will be clearer for people using it, avoid confusion for people needing server functionality, and help organise the code better.

Closes #120

This will fix some confusion, and stop client-only installs from being broken now that `Thing` can be imported at top level.

Closes #120
@barecheck
Copy link
Copy Markdown

barecheck bot commented Jun 28, 2025

Barecheck - Code coverage report

Total: 85.75%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

Copy link
Copy Markdown
Contributor

@julianstirling julianstirling left a comment

Choose a reason for hiding this comment

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

Yes please!

quickstart_example.sh and index.rst both need updating to not ask for [server] install. Otherwise this is ready to go.

@rwb27
Copy link
Copy Markdown
Collaborator Author

rwb27 commented Jun 28, 2025

quickstart_example.sh and index.rst both need updating to not ask for [server] install. Otherwise this is ready to go.

I'm a little surprised that didn't cause the test of quickstart to fail. I guess pip will still have installed it OK, and warnings from the bash script that installs stuff don't get captured...

@rwb27
Copy link
Copy Markdown
Collaborator Author

rwb27 commented Jun 28, 2025

Thanks for spotting it in the docs. That's updated now, and I have searched for [server and not found it anywhere else...

Copy link
Copy Markdown
Contributor

@julianstirling julianstirling left a comment

Choose a reason for hiding this comment

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

Approved. Merge at will

Copy link
Copy Markdown
Contributor

@julianstirling julianstirling left a comment

Choose a reason for hiding this comment

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

Sorry, unapproved... search server] too!

@julianstirling
Copy link
Copy Markdown
Contributor

Rather unhelpful comment there. I was just rushing to stop a merge... pip install -e .[dev,server] in test.yml

@rwb27
Copy link
Copy Markdown
Collaborator Author

rwb27 commented Jul 1, 2025

Sorry about that, and thanks for spotting it. I guess pip only warns about missing extras, so all the tests still passed (same problem as the test that installs it using the quickstart instructions).

I have done a rather more comprehensive search and I believe I've removed any mention of the server extra from the repo. I've also updated the install instructions in the README. There are annoyingly many places where there are install instructions, but hopefully pip install labthings-fastapi is not something we will have to change in the near future. I think single-sourcing that would cause more problems than it solves.

@rwb27 rwb27 merged commit 093adfa into main Jul 2, 2025
14 checks passed
@rwb27 rwb27 deleted the non-optional-server-dependencies branch July 2, 2025 13:46
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.

Client-only installation is broken

2 participants