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

Improve separation between FastAPI and Starlette #20

Merged
merged 4 commits into from
Dec 19, 2020

Conversation

br3ndonland
Copy link
Owner

@br3ndonland br3ndonland commented Dec 19, 2020

Description

FastAPI depends on Starlette. For simplicity, inboard was set up to install FastAPI from the get-go, because installing FastAPI will also install Starlette. However, this means that the Starlette application is dependent on FastAPI, which is sort of backwards. Within the inboard source code, the utilities module contained the HTTP Basic Auth implementations for Starlette and FastAPI, but because FastAPI was imported into that module, it also meant that the Starlette application required FastAPI in order to import the module.

This PR will improve the separation of concerns between FastAPI and Starlette, so that the FastAPI and Starlette features are independent, and the Starlette application can run without installing FastAPI.

Changes

FastAPI 0.62.0 and Starlette 0.14 have mutually incompatible version constraints

As far as I can tell, it's not possible to install both FastAPI 0.62 and Starlette 0.14 in the same Poetry project. FastAPI 0.62.0 has Starlette pinned at 0.13.6. An update to Starlette 0.14 is under consideration, but requires some refactoring (fastapi/fastapi#2335). Attempts to add Starlette 0.14 to inboard directly raise a Poetry SolverProblemError (on Poetry 1.1.4):

❯ poetry add starlette@'^0.14'

Updating dependencies
Resolving dependencies... (0.1s)

  SolverProblemError

  Because fastapi (0.62.0) depends on starlette (0.13.6)
  and no versions of fastapi match >0.62,<0.63, fastapi (>=0.62,<0.63) requires starlette (0.13.6).
  So, because inboard depends on both fastapi (^0.62) and starlette (^0.14), version solving failed.

  at ~/.poetry/lib/poetry/puzzle/solver.py:241 in _solve
      237│             packages = result.packages
      238│         except OverrideNeeded as e:
      239│             return self.solve_in_compatibility_mode(e.overrides, use_latest=use_latest)
      240│         except SolveFailure as e:
    → 241│             raise SolverProblemError(e)
      242│
      243│         results = dict(
      244│             depth_first_search(
      245│                 PackageNode(self._package, packages), aggregate_package_nodes

I would be open to having FastAPI 0.62 and Starlette 0.14 be separate sets of dependencies, but even if only one set of extras is installed (poetry install -E starlette), Poetry appears to resolve all the dependencies together, so the environment will still end up with Starlette 0.13.6.

Docker partial solution

In Docker, FastAPI can be removed from the Poetry project prior to installing Starlette, so that Starlette 0.14 can be installed.

FROM base AS starlette
ENV APP_MODULE=inboard.app.main_starlette:app PATH=$POETRY_HOME/bin:$PATH
RUN poetry remove fastapi && poetry add starlette@^0.14 --optional && \
  poetry install --no-dev --no-interaction --no-root -E starlette

This looks like it would work well, but for some reason, running poetry remove fastapi installs all the dev dependencies, then poetry install --no-dev removes them. Totally counterintuitive and inefficient.

Docker build logs
Step 18/22 : FROM base AS starlette
 ---> edf48237e383
Step 19/22 : ENV APP_MODULE=inboard.app.main_starlette:app PATH=$POETRY_HOME/bin:$PATH
 ---> Using cache
 ---> 7548d5f13393
Step 20/22 : RUN poetry remove fastapi
 ---> Running in 46e65064e023
Skipping virtualenv creation, as specified in config file.
Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 37 installs, 0 updates, 0 removals

  • Installing pyparsing (2.4.7)
  • Installing appdirs (1.4.4)
  • Installing attrs (20.3.0)
  • Installing distlib (0.3.1)
  • Installing filelock (3.0.12)
  • Installing iniconfig (1.1.1)
  • Installing packaging (20.8)
  • Installing pluggy (0.13.1)
  • Installing py (1.10.0)
  • Installing six (1.15.0)
  • Installing toml (0.10.2)
  • Installing certifi (2020.12.5)
  • Installing cfgv (3.2.0)
  • Installing chardet (4.0.0)
  • Installing coverage (5.3)
  • Installing idna (2.10)
  • Installing identify (1.5.10)
  • Installing mccabe (0.6.1)
  • Installing mypy-extensions (0.4.3)
  • Installing nodeenv (1.5.0)
  • Installing pathspec (0.8.1)
  • Installing pycodestyle (2.6.0)
  • Installing pyflakes (2.2.0)
  • Installing pytest (6.2.1)
  • Installing regex (2020.11.13)
  • Installing typed-ast (1.4.1)
  • Installing typing-extensions (3.7.4.3)
  • Installing urllib3 (1.26.2)
  • Installing virtualenv (20.2.2)
  • Installing black (20.8b1)
  • Installing flake8 (3.8.4)
  • Installing isort (5.6.4)
  • Installing mypy (0.790)
  • Installing pre-commit (2.9.3)
  • Installing pytest-cov (2.10.1)
  • Installing pytest-mock (3.4.0)
  • Installing requests (2.25.1)
Removing intermediate container 46e65064e023
 ---> 5b90fecdeca0
Step 21/22 : RUN poetry add starlette@^0.14
 ---> Running in 91d3d1cee3f0
Skipping virtualenv creation, as specified in config file.

Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 1 install, 0 updates, 0 removals

  • Installing starlette (0.14.1)
Removing intermediate container 91d3d1cee3f0
 ---> 76b7b3213c93
Step 22/22 : RUN poetry install --no-dev --no-interaction --no-root -E starlette
 ---> Running in 24aa553a5052
Skipping virtualenv creation, as specified in config file.
Installing dependencies from lock file

Package operations: 0 installs, 0 updates, 37 removals

  • Removing appdirs (1.4.4)
  • Removing attrs (20.3.0)
  • Removing black (20.8b1)
  • Removing certifi (2020.12.5)
  • Removing cfgv (3.2.0)
  • Removing chardet (4.0.0)
  • Removing coverage (5.3)
  • Removing distlib (0.3.1)
  • Removing filelock (3.0.12)
  • Removing flake8 (3.8.4)
  • Removing identify (1.5.10)
  • Removing idna (2.10)
  • Removing iniconfig (1.1.1)
  • Removing isort (5.6.4)
  • Removing mccabe (0.6.1)
  • Removing mypy (0.790)
  • Removing mypy-extensions (0.4.3)
  • Removing nodeenv (1.5.0)
  • Removing packaging (20.8)
  • Removing pathspec (0.8.1)
  • Removing pluggy (0.13.1)
  • Removing pre-commit (2.9.3)
  • Removing py (1.10.0)
  • Removing pycodestyle (2.6.0)
  • Removing pyflakes (2.2.0)
  • Removing pyparsing (2.4.7)
  • Removing pytest (6.2.1)
  • Removing pytest-cov (2.10.1)
  • Removing pytest-mock (3.4.0)
  • Removing regex (2020.11.13)
  • Removing requests (2.25.1)
  • Removing six (1.15.0)
  • Removing toml (0.10.2)
  • Removing typed-ast (1.4.1)
  • Removing typing-extensions (3.7.4.3)
  • Removing urllib3 (1.26.2)
  • Removing virtualenv (20.2.2)
Removing intermediate container 24aa553a5052
 ---> 3b67425ed943
Successfully built 3b67425ed943
Successfully tagged localhost/br3ndonland/inboard:starlette

Possible Poetry solutions

There are some related Poetry features, but none fully address this use case.

The union operator

The docs on dependency specification show a union operator (||) like this:

[tool.poetry.dependencies]
pathlib2 = { version = "^2.2", python = "~2.7 || ^3.2" }

It's unclear how or where the union operator can be used, and it is error-prone. For example, I had a recent PyPI publication fail because of the union operator. I tried constraining the Starlette dependency like this:

[tool.poetry.dependencies]
starlette = {version = "0.13.6 || ^0.14", optional = true}

Poetry 1.1.4 installs and builds the project, and poetry check returns All set!, but poetry publish fails to upload to PyPI because of the union operator in the version constraint for Starlette.

GitHub Actions logs show:

Building inboard (0.7.0)
  - Building sdist
  - Built inboard-0.7.0.tar.gz
  - Building wheel
  - Built inboard-0.7.0-py3-none-any.whl

Publishing inboard (0.7.0) to PyPI
 - Uploading inboard-0.7.0-py3-none-any.whl 0%
 - Uploading inboard-0.7.0-py3-none-any.whl 100%

  UploadError

  HTTP Error 400: Invalid value for requires_dist. Error:
  Invalid requirement:
  'starlette (0.13.6 || >=0.14,<0.15); extra == "starlette"'.

  at ~/.poetry/lib/poetry/publishing/uploader.py:216 in _upload
      212│                     self._register(session, url)
      213│                 except HTTPError as e:
      214│                     raise UploadError(e)
      215│
    → 216│             raise UploadError(e)
      217│
      218│     def _do_upload(
      219│         self, session, url, dry_run=False
      220│     ):  # type: (requests.Session, str, Optional[bool]) -> None

Additionally, union operators may not work properly with multiple-constraint dependencies (python-poetry/poetry#2340).

Specifying multiple version constraints with python or markers

"Markers" are constraints used when installing Python packages. They are mostly for system-level constraints, like operating system and Python version, and don't seem like they can be used to specify groups of dependencies. There's no marker for FastAPI, so markers don't satisfy the use case here.

Markers are supported by Poetry, though they are only partially documented. If you would like to learn more about markers, and don't mind confusing yourself, you can read through the opaque "parsley grammar" in PEP 508.

Related:

pyproject.toml examples with multiple constraints

Based on tests/fixtures/project_with_multi_constraints_dependency/pyproject.toml.

# pyproject.toml without extras
[tool.poetry]
name = "project-with-multi-constraints-dependency"
version = "1.2.3"
description = "This is a description"
authors = ["Your Name <[email protected]>"]
license = "MIT"

[tool.poetry.dependencies]
python = "*"
pendulum = [
    { version = "^1.5", python = "<3.4" },
    { version = "^2.0", python = "^3.4" }
]

[tool.poetry.dev-dependencies]
# pyproject.toml with extras
[tool.poetry]
name = "project-with-multi-constraints-dependency"
version = "1.2.3"
description = "This is a description"
authors = ["Your Name <[email protected]>"]
license = "MIT"

[tool.poetry.dependencies]
python = "*"
pendulum = [
    {version = "^1.5", python = "<3.4", optional = true},
    {version = "^2.0", python = "^3.4", optional = true}
]

[tool.poetry.dev-dependencies]

[tool.poetry.extras]
time = ["pendulum"]

Using a wildcard in the tool.poetry.dependencies section, and then using tool.poetry.extras to specify versions

I tried this out, but it doesn't work. Specifying version constraints for extras in pyproject.toml, then running poetry update, deletes the extras from poetry.lock.

pyproject.toml example with version constraints in extras
# pyproject.toml with version constraints in extras
[tool.poetry]
name = "poetrydependencyversions"
version = "0.0.1"
description = "Installing multiple versions of a dependency with Poetry."
authors = ["Your Name <[email protected]>"]
license = "MIT"

[tool.poetry.dependencies]
python = "^3.8"
fastapi = {version = "*", optional = true}
starlette = {version = "*", optional = true}

[tool.poetry.extras]
# these version constraints don't work as expected
fastapi = ["starlette ==0.13.6", "fastapi"]
starlette = ["starlette >=0.14.0"]

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

Confusingly, running poetry check over the pyproject.toml returns an "All set!" success message, even though it's clearly not behaving as expected.

This issue may be addressed in upcoming releases (python-poetry/poetry-core#103).

81d5a90
a3ffa71
823408d
c40a020
9c79964

This commit will separate the utilities for FastAPI and Starlette into
separate modules, so that the Starlette application can run if FastAPI
is not installed.
@codecov
Copy link

codecov bot commented Dec 19, 2020

Codecov Report

Merging #20 (bbd9e61) into develop (7236cd5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop       #20   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         9    +1     
  Lines          219       221    +2     
=========================================
+ Hits           219       221    +2     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
inboard/app/utilities_starlette.py 100.00% <ø> (ø)
inboard/app/main_fastapi.py 100.00% <100.00%> (ø)
inboard/app/main_starlette.py 100.00% <100.00%> (ø)
inboard/app/utilities_fastapi.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7236cd5...bbd9e61. Read the comment docs.

As far as I can tell, it's not possible to install both FastAPI 0.62 and
Starlette 0.14 in the same Poetry project. FastAPI 0.62.0 has Starlette
pinned at 0.13.6, and attempts to add Starlette 0.14 raise a Poetry
`SolverProblemError` (on Poetry 1.1.4):

```text
❯ poetry add starlette@'^0.14'

Updating dependencies
Resolving dependencies... (0.1s)

  SolverProblemError

  Because fastapi (0.62.0) depends on starlette (0.13.6)
  and no versions of fastapi match >0.62,<0.63, fastapi (>=0.62,<0.63)
  requires starlette (0.13.6).
  So, because inboard depends on both fastapi (^0.62) and starlette
  (^0.14), version solving failed.

  at ~/.poetry/lib/poetry/puzzle/solver.py:241 in _solve
      237│             packages = result.packages
      238│         except OverrideNeeded as e:
      239│             return self.solve_in_compatibility_mode(e.overrides, use_latest=use_latest)
      240│         except SolveFailure as e:
    → 241│             raise SolverProblemError(e)
      242│
      243│         results = dict(
      244│             depth_first_search(
      245│                 PackageNode(self._package, packages), aggregate_package_nodes
```

I would be open to having FastAPI 0.62 and Starlette 0.14 be separate
sets of dependencies, but even if only one set of extras is installed
(`poetry install -E starlette`), Poetry appears to resolve all the
dependencies together, so the environment will still end up with
Starlette 0.13.6.

The easiest solution is to just install the Starlette version pinned by
FastAPI (0.13.6), but add a Poetry union operator (`||`) so upgrading
to Starlette 0.14 is at least possible in the future. The order of the
union doesn't seem to matter.

In Docker, FastAPI can be removed from the Poetry project prior to
installing Starlette, so that Starlette 0.14 can be installed.
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 19, 2020

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 7.81%.

Quality metrics Before After Change
Complexity 1.51 ⭐ 0.21 ⭐ -1.30 👍
Method Length 32.87 ⭐ 27.08 ⭐ -5.79 👍
Working memory 7.88 🙂 6.41 🙂 -1.47 👍
Quality 79.98% 87.79% 7.81% 👍
Other metrics Before After Change
Lines 135 83 -52
Changed files Quality Before Quality After Quality Change
inboard/app/main_fastapi.py 86.36% ⭐ 86.36% ⭐ 0.00%
inboard/app/main_starlette.py 88.96% ⭐ 88.96% ⭐ 0.00%

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it by mentioning @sourcery-ai in a comment.

@br3ndonland br3ndonland merged commit e1e34c7 into develop Dec 19, 2020
@br3ndonland br3ndonland deleted the fastapi!=starlette branch December 19, 2020 12:52
br3ndonland added a commit that referenced this pull request Dec 20, 2020
This reverts commit bbd9e61.
#20
https://python-poetry.org/docs/dependency-specification/

Poetry 1.1.4 installs and builds the project, and `poetry check` returns
`All set!`, but `poetry publish` fails to upload to PyPI because of the
union operator in the version constraint for Starlette.

The docs on dependency specification show the union operator like this:

```toml
[tool.poetry.dependencies]
pathlib2 = { version = "^2.2", python = "~2.7 || ^3.2" }
```

and mine looks like this:

```toml
[tool.poetry.dependencies]
starlette = {version = "0.13.6 || ^0.14", optional = true}
```

It may be that the union operator can only be used in the `python`
constraint. I'm just going to remove it.

GitHub Actions: https://github.com/br3ndonland/inboard/runs/1583331395

```text
Building inboard (0.7.0)
  - Building sdist
  - Built inboard-0.7.0.tar.gz
  - Building wheel
  - Built inboard-0.7.0-py3-none-any.whl

Publishing inboard (0.7.0) to PyPI
 - Uploading inboard-0.7.0-py3-none-any.whl 0%
 - Uploading inboard-0.7.0-py3-none-any.whl 100%

  UploadError

  HTTP Error 400: Invalid value for requires_dist. Error:
  Invalid requirement:
  'starlette (0.13.6 || >=0.14,<0.15); extra == "starlette"'.

  at ~/.poetry/lib/poetry/publishing/uploader.py:216 in _upload
      212│                     self._register(session, url)
      213│                 except HTTPError as e:
      214│                     raise UploadError(e)
      215│
    → 216│             raise UploadError(e)
      217│
      218│     def _do_upload(
      219│         self, session, url, dry_run=False
      220│     ):  # type: (requests.Session, str, Optional[bool]) -> None

```
br3ndonland added a commit that referenced this pull request Apr 19, 2021
#20
#32
#33

This commit will refine the imports in inboard/`__init__.py` so that
FastAPI and Starlette are imported separately. This way, if Starlette is
installed but FastAPI is not, the HTTP Basic auth implementation for
Starlette can still be imported and added to `__all__`.
br3ndonland added a commit that referenced this pull request May 20, 2021
#20
https://github.com/tiangolo/fastapi/releases/tag/0.65.0

FastAPI 0.65 now supports Starlette 0.14, which unblocks the issue
described in #20, bbd9e61, and 12eef2a.
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