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

FastAPI dependency makes difficult to use new fastapi-slim #29

Closed
hasier opened this issue Jun 4, 2024 · 3 comments · Fixed by #30
Closed

FastAPI dependency makes difficult to use new fastapi-slim #29

hasier opened this issue Jun 4, 2024 · 3 comments · Fixed by #30

Comments

@hasier
Copy link
Contributor

hasier commented Jun 4, 2024

Problem

With the inclusion of the new fastapi-slim package, it means fastapi now includes all the extra dependencies FastAPI does not strictly need. We are looking at using fastapi-slim and manually adding only the dependencies we need. However, as fastapi-injector depends on fastapi directly, we cannot really do that.

Possible solutions

  • Because fastapi does not even depend on fastapi-slim, it'd be weard to tweak the dependency to fastapi-slim, as their projects would not directly resolve to fastapi, and they'd end up installing both. This probably has no impact in the final bundle, but it does not look like the tidiest outcome.
  • We could tweak fastapi-injector to not define either as a direct dependency, only keep one of them as optional, and let the users choose whichever they prefer, similar to what the OpenTelemetry packages do.

Let me know if any of these (or any other ideas) are interesting to you, happy to work on a PR to get it done. Thanks for your time and effort!

@hasier hasier changed the title a FastAPI dependency makes difficult to use new fastapi-slim Jun 4, 2024
@hasier
Copy link
Contributor Author

hasier commented Jun 4, 2024

Sorry, submitted inadvertently with no contents 🤦 Just amended the title and description.

@matyasrichter
Copy link
Owner

Hi, thanks for bringing this up.

I have to say that the way they chose to implement this is a bit annoying. Doesn't it break every single library that depends on fastapi in this exact way?

I'd be fine with what you proposed - making it an optional dependency.
The issues I can think of are:

  1. We cannot enforce any version constraints on fastapi unless the user asks for the extra dependency. IIRC, we don't need this right now (I'm not actually sure how I chose the current constraint), but it could be useful one day...
  2. It would cause a crash if a user of fastapi-injector forgets to install fastapi on their own. But I guess it's reasonable to expect that they already have it.
    I can't think of any solutions, though. I thought maybe we could specify two extras groups, one with each fastapi version, and make one of them "default", but a quick search did not bring up a way to specify that in pyproject.toml. So we probably just need to make a compromise, if we want to make fastapi-injector usable with fastapi-sim.

If you could make a PR, that would be great.

@hasier
Copy link
Contributor Author

hasier commented Jun 4, 2024

Thanks for looking into this so quickly!

Hi, thanks for bringing this up.

I have to say that the way they chose to implement this is a bit annoying. Doesn't it break every single library that depends on fastapi in this exact way?

Yeah agree, it probably does not break any project, but it could inadvertently add a bunch of new irrelevant dependencies...

I'd be fine with what you proposed - making it an optional dependency. The issues I can think of are:

  1. We cannot enforce any version constraints on fastapi unless the user asks for the extra dependency. IIRC, we don't need this right now (I'm not actually sure how I chose the current constraint), but it could be useful one day...

We can at least set the minimum versions even if they are optional so that they are explicit. I think it would also enforce them in the projects where they are used, at least when I used Poetry it'd complain if the optional ranges would not allow to resolve the right sub-dependencies, so maybe it prevents using it when the versions from the project don't align?

  1. It would cause a crash if a user of fastapi-injector forgets to install fastapi on their own. But I guess it's reasonable to expect that they already have it.

I guess that's reasonable too.

I can't think of any solutions, though. I thought maybe we could specify two extras groups, one with each fastapi version, and make one of them "default", but a quick search did not bring up a way to specify that in pyproject.toml. So we probably just need to make a compromise, if we want to make fastapi-injector usable with fastapi-sim.

I think it'd make sense to create 2 groups, we cannot make one a default, but hopefully by reading the names it is quite straightforward, something like standard and slim.

If you could make a PR, that would be great.

Just opened #30, happy to hear your thoughts 🙂

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 a pull request may close this issue.

2 participants