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

Fixed FastAPIServer uvicorn path #15

Closed
wants to merge 5 commits into from
Closed

Conversation

LeOndaz
Copy link
Contributor

@LeOndaz LeOndaz commented Aug 11, 2021

No description provided.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Aug 11, 2021

@mrichar1 the paths were on the following format

uvicorn __path__/app.py:app --host ... --port ...

and I removed the .py here.

@mrichar1
Copy link
Member

Thanks for fixing this. For completeness we probably need tests and a sample_app for this to ensure everything runs as expected in future if there are any changes to the guvicorn/fastapi interfaces.

The steps for this are:

  1. Add a fastapi sample app in sample_apps (based on the others).
  2. Update test_examples/unittest_example/tests.py to test FastAPI (based on the others).
  3. Update test_examples/pytest_example/tests.py in the same way.

You should then be able to run tox and have everything pass.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Aug 11, 2021

@mrichar1 Sure, however, I don't really have the time now, if this looks good, you can merge and I'll PR the tests and an example within the next 5-7 days.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Aug 25, 2021

@mrichar1 Hello, sorry for being late. I currently won't be able to push this because my machine got ruined. I'll be spending some time before I get a new one. I can't test the current config atm. If you would like to merge or continue from here it will be appreciated, however, if you don't. I'll not disappear forever.

Thanks for understanding & have a great day.

@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 11, 2021

@mrichar1 Hello, I've came back from my deep coma, I'm testing in the repo because tests don't work locally and I'm lazy to debug

@LeOndaz LeOndaz force-pushed the master branch 2 times, most recently from 00ca25b to 86e174e Compare October 11, 2021 01:33
@LeOndaz
Copy link
Contributor Author

LeOndaz commented Oct 11, 2021

@mrichar1 Have a look when you have time, this is the command that ran

uvicorn test_examples/unittest_example/../../sample_apps/fastapi/main:app --host 127.0.0.1 --port 8001 --reload

and for some reason it's not working, I'll wait for you and thanks anyway.

@mrichar1
Copy link
Member

mrichar1 commented Nov 24, 2021

I've just had a quick look at this - the tests are failing as the startup methods for uvicorn needs the app path and app name split out.

On the command-line this would be:

uvicorn --app-dir /home/mrichar1/git/liveandletdie/test_examples/unittest_example/../../sample_apps/fastapi/ main:app

This will need fixed here:

https://github.com/LeOndaz/liveandletdie/blob/master/liveandletdie/__init__.py#L571-L577

To be something like:

def create_command(self):
        path = os.path.dirname(self.path)

        return [
            self.executable,
            '--app-dir',
            path,
            'main:app',
            '--host',
            self.host,
            '--port',
            self.port,
            '--reload',
        ]

Additionally, from looking at the uvicorn docs, it looks like request.url will be an object, so here:

https://github.com/LeOndaz/liveandletdie/blob/86e174eda1a3a1ab810d16d1e3a85d0aff13dc55/sample_apps/fastapi/main.py#L10

You need to instead do:

if request.url.scheme == 'https':

that should get the tests passing, though you'll probably want to also test that the instance of uvicorn that liveandletdie is starting is correctly configured for real-world usage.

sample_apps/fastapi/main.py Outdated Show resolved Hide resolved
@jensens
Copy link
Member

jensens commented Jun 9, 2023

followup at #16

@jensens jensens closed this Jun 9, 2023
Copy link
Contributor Author

@LeOndaz LeOndaz left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

3 participants