✨ Add `--factory` option

[rasputyashka]

I have seen sort of the same implementation in one of pull requests, but I didn't like it, so I've written my own, simpler one.

I haven't written any tests, but I have some spare time to do so if needed. I am not sure if the help messages look good, but I can consider thinking of it if you want to merge this request (or you can do it yourself).

[github-actions[bot]]

This pull request has a merge conflict that needs to be resolved.

[YuriiMotov]

@rasputyashka, thanks for working on this!

This feature looks highly requested: https://github.com/fastapi/fastapi-cli/discussions/45

Could you please resolve merge conflicts and add tests? I would then review this PR

[rasputyashka]

@YuriiMotov yeah, sure.

[rasputyashka]

it tuned out that the only thing that should've been added is the factory argument to the cli.

(btw uvicorn can detect whether passed object is a factory and call it, but it still recommends setting the factory flag explicitly).

so recommended way of running an app with factory is fastapi run -e path.to.module:factory --factory

[YuriiMotov]

We can also run the app as fastapi run path/to/module.py --app app. And this way still doesn't work with factory.

How about enabling this too? Seems that the following change in discover.py/get_app_name helps:

-        if not isinstance(app, FastAPI):
+        if (not isinstance(app, FastAPI)) and (not inspect.isfunction(app)):
            raise FastAPICLIException(
-                f"The app name {app_name} in {mod_data.module_import_str} doesn't seem to be a FastAPI app"
+                f"The app name {app_name} in {mod_data.module_import_str} doesn't seem to be a FastAPI app or factory"
            )

Surprisingly, we can run server passing factory function that returns string (instead of FastAPI app) and it will not complain until it tries to handle first request.. But, it works the same way with Uvicorn, so I'm not sure we can handle this now

[rasputyashka]

I feel like there's need to rename '--app' argument to something like 'candidate' because factory is not an app (and i do NOT like this idea), that's why i decided not to implement --factory support for --app argument. (I mean user would do something like fastapi run path/to/module.py --app function --factory. That just looks odd)

As of string behavior, I think uvicorn should handle these cases since we need to get the returned value (call the factory), check if this is an instance or a subclass of a FastAPI class (this would cause confusion during debugging)

[YuriiMotov]

I actually don't see any problem with --app option accepting app factory. But, let's wait for third opinion :)

I think we can limit this PR to only support --factory flag for --entrypoint use case. IMO, this PR will be still valuable. But in this case we need to:

• Raise "--factory option is only supported with --entrypoint" error if --entrypoint is not specified
• Remove and app factory from the messages related to running server without --entrypoint specified

[rasputyashka]

Well, the --app issue sounds more like a naming issue for me, that's why I'd like to wait for the owner\contributor (all these fixes require owner's interventions)

I'm OK with having this MR as an entrypoint's option but not sure if this is all we can do for now (since patching current implementation is easy and quick). Let's wait for anybody who can merge the changes to hear their opinion.

[YuriiMotov]

@rasputyashka, could you please update the description to make it simpler for Sebastian to review? You can summarize what is done and what is under the question. Then we could hide some comments as resolved

[rasputyashka]

To sum up:

Extra arg to the cli (--factory) was added (works only with --entrypoint argument).

Recommended way of running an app with factory is fastapi run -e path.to.module:factory --factory

I considered not to add factory argument to the use case when user runs

fastapi run path/to/module.py --app instance_of_fastapi

because --app means that user passes an app, not a function that produces an app (naming issue). (we can add another argument --app-factory that would do the same as --app but will used in context when function is expected). Ignoring this issue and saying that factory is an app is an option as well :D

@YuriiMotov suggested this to improve current behavior:

• Raise "--factory option is only supported with --entrypoint" error if --entrypoint is not specified
• Remove and app factory from the messages related to running server without --entrypoint specified\

[YuriiMotov]

I thought a bit more and I think suggested --app-factory option is nice solution! And, with --app-factory users will not have to specify --factory since we already know that it's a factory. We should make --app-factory mutually exclusive with --app option (I mean just adding a condition and print a warning\error).

[rasputyashka]

I do like the idea, let's see what maintainers think about this

[YuriiMotov]

It's up to you, but I would recommend you edit this PR to make it ready to be merged. It can be simple version that only supports --factory with --entrypoint. Or the version we discussed above. If PR is approved by one of team members, it will have much more chances to be reviewed by Sebastian soon