Import testing dynamically, install requests as extra dependency

[Bobronium]

Closes #130

>>> from starlite import Starlite
>>> Starlite
<class 'starlite.app.Starlite'>
>>> from starlite import TestClient
Traceback (most recent call last):
    ...
MissingDependencyException: To use starlite.testing, install starlite with 'testing' extra, e.g. `pip install starlite[testing]`

[peterschutt]

Have we considered the option of vendoring in this RequestEncodingMixin class and do away with the requests dependency altogether? The whole class is ~120 lines including docstrings, and mainly relies on stdlib except for a couple of small helpers.

https://github.com/psf/requests/blob/da9996fe4dc63356e9467d0a5e10df3d89a8528e/requests/models.py#L84

The only people that would break is then anyone relying on requests via our dependency without listing it in their own dependencies, which wouldn't be many as I think most would be using an async client, and not really our problem anyway.

[vrslev]

Starlite has several potentially optional dependencies: pydantic-factories for generating examples in OpenAPI schema, pyyaml for /openapi.yaml path and—of course—requests for testing. If we decide that requests is optional, we should consider to make pydantic-factories and pyyaml not required as well.

[peterschutt]

Thanks @vrslev - makes sense to look at all that together indeed.

If you look at #165 I'm considering using package extras for centralizing dev dependency specification across poetry and tox, so at least for the requests one - if we have to keep it (which I'm pretty sure we don't) - there'd be a test extras for it to slot into there anyway.

If you are playing around with the codebase I'd be interested in how you go with the workflow that I'm building there for testing locally across python versions.

Do you have a pattern that you use for that sort of thing independent of this project?

[vrslev]

@peterschutt If you want reproducible workflow for testing multiple Python versions, you'd have to use tox or similar tools. Also nox and hatch are worth mentioning.
Personally, I work on latest Python version that a project supports and track language Python version problems with CI and Pyright type checker (it has pythonVersion option and reports when something wouldn't work on previous versions). I don't see any significant advantage of being able to easily test a project locally on all supported Pythons.

[peterschutt]

Yep, using tox.

I'm certainly not running tests across all versions while I'm iterating over code, but I much prefer to find any errors locally before having to wait on CI to find them.

Cheers for the input.

[Goldziher]

@Bobronium -- very nice PR. Please look into why there is a test failure on v3.10.

@peterschutt whats your suggestion regarding vendoring? I don't quite understand.

[Goldziher]

interesting, I have never seen this used before.

So this will prevent a breaking change by keeping the import from starlite. The problem is how to prevent users from continuing to import from starlite in the future - I assume the IDE will not try to import from here by default, correct?

[Bobronium]

At first, I also raised deprecation warning, as well as removed these names from __all__. But in c81d99a I thought, why forbid it if we can handle it gracefully anyway?

Currently, IDE will still import it from starlite package, since it's declared in __all__ and imported in if TYPE_CHECKING block — so from IDE's perspective not much has changed.

If we decide that we want to discourage it though, then removing the names from __all__ and if TYPE_CHECKING block would be sufficient for redirecting auto-import to starlite.testing.

[Goldziher]

Ok, Im not 100% sure I like this, perhaps deprecations and discouragement are better -- @peterschutt whats your opinion here?

[Bobronium]

@Goldziher, regarding vendoring: as I understood it, @peterschutt proposed copy-pasting needed code from requests lib to starlite source code, since we borrow only small amount from it.

I actually think it makes perfect sense, if we're not using 99% of what comes with requests dependency.

[Goldziher]

@Goldziher, regarding vendoring: as I understood it, @peterschutt proposed copy-pasting needed code from requests lib to starlite source code, since we borrow only small amount from it.

I actually think it makes perfect sense, if we're not using 99% of what comes with requests dependency.

I see. But thats not actually correct - we also build on the Starlette testing package, which relies on requests extensively. So in either case we are going to have requests around.

[vrslev]

You can set starite to None and achieve the same result. Example: https://github.com/vrslev/ikea-api-client/blob/1b9003111ed774751705ed14076dbb0882a24bd0/tests/test_init.py

[vrslev]

Source code for the test: https://github.com/vrslev/ikea-api-client/blob/1b9003111ed774751705ed14076dbb0882a24bd0/src/ikea_api/__init__.py

[Bobronium]

For some reason, It worked If I ran test separately, but not when they all ran with pytest. I tried importlib.reload() as well.

Having a clean subprocess is slower, but more right in terms of what we expect to get. Reloading modules when they already were imported somewhere may cause weird bugs when some_module.reloaded_module.Class is not reloaded_module.Class if some_module imported reloaded_module before it was reloaded and check is happening after.

[Bobronium]

To be more specific:

reloaded_module.py

class Class:
    pass

some_module.py

from reloaded_module import Class

instance = Class()

main.py

import sys
import reloaded_module
import some_module

assert isinstance(some_module.instance, reloaded_module.Class), "Before reload"

del sys.modules["reloaded_module"]
import reloaded_module

assert isinstance(some_module.instance, reloaded_module.Class), 'After reload'

Running main.py will result in

Traceback (most recent call last):
  File "main.py", line 10, in <module>
    assert isinstance(some_module.instance, reloaded_module.Class), 'After reload'
AssertionError: After reload

[vrslev]

I don’t think it matters because tests are isolated from each other 🤔

[Bobronium]

I wouldn't say tests are really isolated. To some degree, maybe. But all modules are still shared.

Tests are collected first, then executed. Almost all of the imports are done during collection phase.
If then one module is reloaded, there still might be a conflict between what's already imported in other tests modules later in execution.

If it's still unclear what I mean, I'm pretty sure I can come up with another example.

[vrslev]

No, I get it, thanks :)

[Bobronium]

@Goldziher, CI is still failing, but I think it's unrelated to the changes:

`snyk` requires an authenticated account. Please run `snyk auth` and try again.

[Goldziher]

@Goldziher, CI is still failing, but I think it's unrelated to the changes:

`snyk` requires an authenticated account. Please run `snyk auth` and try again.

yes, its not because of you.

[Goldziher]

please rebase

[peterschutt]

I see. But thats not actually correct - we also build on the Starlette testing package, which relies on requests extensively.

No, that's not correct. Requests is a dependency of the full extra for starlette, which we don't install. The only dependencies of starlette itself are anyio and typing-extensions: https://github.com/encode/starlette/blob/1225ae8404ed0ae0b9170fde1cf824ec3f9c0f2f/setup.py#L39-L42

So, if we are relying on functionality in starlette that requires requests but are relying on requests as a side-effect via our own dependency - just so we can depend on this one single utility class, then that is very untidy IMO.

So in either case we are going to have requests around.

We have hundreds of transitive dependencies, that doesn't justify this change IMO.

I have utmost respect for the work put in here, but the complexity to payoff ratio of this PR is way off IMO. I'm 100% sure we should be just vendoring that class in, in a private module so that it is obviously not for downstream users, and get rid of requests altogether from our own dependencies. If starlette need it for their testing things, then we rely on their full extras and let them manage the dependency.

[Goldziher]

I see. But thats not actually correct - we also build on the Starlette testing package, which relies on requests extensively.

No, that's not correct. Requests is a dependency of the full extra for starlette, which we don't install. The only dependencies of starlette itself are anyio and typing-extensions: https://github.com/encode/starlette/blob/1225ae8404ed0ae0b9170fde1cf824ec3f9c0f2f/setup.py#L39-L42

So, if we are relying on functionality in starlette that requires requests but are relying on requests as a side-effect via our own dependency - just so we can depend on this one single utility class, then that is very untidy IMO.

So in either case we are going to have requests around.

We have hundreds of transitive dependencies, that doesn't justify this change IMO.

I have utmost respect for the work put in here, but the complexity to payoff ratio of this PR is way off IMO. I'm 100% sure we should be just vendoring that class in, in a private module so that it is obviously not for downstream users, and get rid of requests altogether from our own dependencies. If starlette need it for their testing things, then we rely on their full extras and let them manage the dependency.

Ok, fair enough. Let's switch to this solution in that case. Are you up to it @Bobronium ?

[Bobronium]

Are you up to it @Bobronium ?

Sorry, up to what exactly?

[Goldziher]

Are you up to it @Bobronium ?

Sorry, up to what exactly?

I was wondering if you'd like to implement that. Or not.

[Bobronium]

I was wondering if you'd like to implement that. Or not.

TL;DR: no :)

For further details, please see: #179

[peterschutt]

Guys, #179 isn't the right solution. My initial enthusiasm didn't factor in all of the urllib3 deps, I read them as stdlib imports when I looked while first forming my opinion. It wouldn't be so bad if we could depend on urllib3 instead of requests and then just vendor in the requests class, but then that doesn't solve this:

we also build on the Starlette testing package, which relies on requests extensively

As without requests, that doesn't work anymore and we don't rely on starlette's full extras which has requests in it. I'd also argue that we don't want to rely on the full extras also.

I think the "right" solution to this is that the testing utilities aren't brought into the top-level namespace at all and then when someone does from starlite.testing import TestClient and they haven't installed the testing extras, there is an informative error. But that's a 2.0 kind of breaking change, so won't fix overnight.

So lets merge this in, and I hope we can move toward a 2.0 where in the starlite namespace there is only Starlite, Router, Controller, get, put, patch, post and delete. That would allow us to do a nicer solution for this problem.

[Bobronium]

Ok, that happened fast.

We didn't address #174 (comment) before merging, so please be aware that IDEs still gonna autoimport TestClient, etc. from starlite.

[peterschutt]

@Bobronium apologies, I missed that - no release has been made so we can always amend/walk back.

if we decide that we want to discourage it though, then removing the names from all and if TYPE_CHECKING block would be sufficient for redirecting auto-import to starlite.testing.

It seems like the obvious fix, but what are the implications? E.g., say I have from starlite import TestClient in my tests, I'll start getting warnings from the IDE that the TestClient doesn't exist there, right, also mypy etc? However, runtime is OK, correct?

[peterschutt]

So if I have from starlite.testing import TestClient I get:

ImportError while loading conftest '/workspace/tests/conftest.py'.
tests/conftest.py:7: in <module>
    from starlite.testing import TestClient
/usr/local/lib/python3.10/site-packages/starlite/testing.py:13: in <module>
    raise MissingDependencyException(
E   starlite.exceptions.MissingDependencyException
ERROR: 4

With from starlite import TestClient I get:

ImportError while loading conftest '/workspace/tests/conftest.py'.
tests/conftest.py:5: in <module>
    from starlite import CacheConfig, Starlite, TestClient
/usr/local/lib/python3.10/site-packages/starlite/__init__.py:139: in __getattr__
    from . import testing
/usr/local/lib/python3.10/site-packages/starlite/testing.py:13: in <module>
    raise MissingDependencyException(
E   starlite.exceptions.MissingDependencyException
ERROR: 4

Just to make sure I'm on the same page. I get no errors from mypy or IDE, and that is because we have TestClient named in __all__ / if TYPE_CHECKING? If we remove those names from __all__ and if TYPE_CHECKING then users will get IDE/type checker warnings about using it from the top-level which would act as a sort of deprecation - Is that the crux of it? LMK if I'm missing anything.

Also a shame that pytest seems to be stripping out the error message pointing to the extra install requirement.

[Bobronium]

It seems like the obvious fix, but what are the implications? E.g., say I have from starlite import TestClient in my tests, I'll start getting warnings from the IDE that the TestClient doesn't exist there, right, also mypy etc? However, runtime is OK, correct?

If requests are installed, correct. if TYPE_CHECKING doesn't affect runtime. __all__ may affect runtime in some cases, e.g. if TestClient was imported with * import (from starlite import *)

Just to make sure I'm on the same page. I get no errors from mypy or IDE, and that is because we have TestClient named in all / if TYPE_CHECKING? If we remove those names from all and if TYPE_CHECKING then users will get IDE/type checker warnings about using it from the top-level which would act as a sort of deprecation - Is that the crux of it

Exactly.

[peterschutt]

if TestClient was imported with * import (from starlite import *)

Yeh ok, I'm kinda OK with the tests breaking - can just add startlite = { extras = ["testing"], ... } to dev extras and get on with your life... but the * import thing is the deciding factor for me. Obviously it's not encouraged for anything meaningful, but when I want to try a library out, I'll install it into a fresh env, run python repl and do from lib import * and have a play around. If just doing that raised an error it would freak me out a little about the lib.

So if the proposed fix for that is removing .testing imports from __all__ then I think that makes sense. I think we should be explicitly deprecating too, and not just relying on the lint warnings.

Deprecating also makes me feel overall better about this as a solution as at least I know there is a sunset clause on how long we have to keep it.

Just a comment: I really don't think there is a net upside with having so much imported in starlite.__init__.py.

[peterschutt]

@Bobronium FYI I've opened a new issue for this seeing that everything has been closed off already.

Thanks for your work on this, up to you or not if you want to pick up this extra stuff. Just LMK either way as I'll work on it otherwise.

[Bobronium]

@peterschutt, I think it makes sense to mention this PR in the issue you've created, this will create a backlink that will be visible here. For history and convenience :)

[peterschutt]

@peterschutt, I think it makes sense to mention this PR in the issue you've created, this will create a backlink that will be visible here. For history and convenience :)

good point, cheers