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

feat: add fastapi docker #2740

Merged
merged 3 commits into from
Mar 3, 2021
Merged

feat: add fastapi docker #2740

merged 3 commits into from
Mar 3, 2021

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Feb 28, 2021

Closes #2615

@Kludex Kludex requested a review from a team as a code owner February 28, 2021 21:01
@Kludex Kludex marked this pull request as draft February 28, 2021 21:01
@Kludex Kludex marked this pull request as ready for review February 28, 2021 23:55
@Kludex
Copy link
Contributor Author

Kludex commented Mar 1, 2021

I'm facing issues to debug it. 😢
Can someone help me? I don't have much experience on this setup 😞

image

If helps:

➜  vscode-docker git:(feat/fastapi-docker) ✗ node --version
v15.10.0
➜  vscode-docker git:(feat/fastapi-docker) ✗ npm --version 
7.5.3

@bwateratmsft
Copy link
Collaborator

I'm facing issues to debug it. 😢
Can someone help me? I don't have much experience on this setup 😞

image

If helps:

➜  vscode-docker git:(feat/fastapi-docker) ✗ node --version
v15.10.0
➜  vscode-docker git:(feat/fastapi-docker) ✗ npm --version 
7.5.3

Could be the same issue as what's causing #2498? There's a bug in VSCode somewhere that causes terminals to not work. Seems like a very active issue (100+ comments): microsoft/vscode#105446

@Kludex
Copy link
Contributor Author

Kludex commented Mar 1, 2021

I'm going to check that in some hours. Thank you!

Do I need to add specific tests? For Django and Flask it looks like they are handled by "python containers" instead of something specific. Am I wrong?

@bwateratmsft
Copy link
Collaborator

Unfortunately we have pretty little in the way of automated tests, and most of what we do have makes more noise than signal 😞

So nothing in particular to add for now.

@Kludex
Copy link
Contributor Author

Kludex commented Mar 1, 2021

Yeah... I've tried some stuff, also downloaded insiders. But it didn't work.

I'm not going to have time to check it again before the weekend, jfyk @bwateratmsft

@bwateratmsft
Copy link
Collaborator

No problem. I'll try to find some time to test these changes out.

@bwateratmsft
Copy link
Collaborator

Ok, I was able to test it out, bumped into a couple of issues.

  1. I need to revert Remove Python code to override entrypoint #2732 and fix it a different way. My bad!
  2. The grouping logic for the "Now listening" regex wasn't right; non-capturing groups did not work as I'd expected. Also I had a typo (missing parenthesis, fixed it above). This is the correct thing to have:
    case 'fastapi':
        return 'Uvicorn running on (https?://\\S+|[0-9]+)';

The reason we don't need a listener for the Gunicorn approach is that when debugging--which is the only time this code will be active--we'll always be using Uvicorn to launch (more on that next...)

  1. Similar to Flask, rather than running the file, we have to run the Uvicorn module in the docker-run task, with arguments pointing to the entrypoint. So, instead of the task being this:
        {
            "type": "docker-run",
            "label": "docker-run: debug",
            "dependsOn": [
                "docker-build"
            ],
            "python": {
                "args": [
                    "--host",
                    "0.0.0.0",
                    "--port",
                    "8000"
                ],
                "file": "main.py"
            }
        }

It needs to be this:

        {
            "type": "docker-run",
            "label": "docker-run: debug",
            "dependsOn": [
                "docker-build"
            ],
            "python": {
                "args": [
                    "main:app",
                    "--host",
                    "0.0.0.0",
                    "--port",
                    "8000"
                ],
                "module": "uvicorn"
            }
        }

The code for generating this task is here, and it contains an example of how the Flask task gets some custom handling--you'd have something very similar, like:

// ... (the Flask code)
} else if (options.projectType === 'fastapi') {
    runOptions.args.unshift(`${path.basename(runOptions.file, '.py')}:app`); // Also, an `import * as path from 'path';` at the top

    runOptions.module = 'uvicorn';
    runOptions.file = undefined;
}

@Kludex
Copy link
Contributor Author

Kludex commented Mar 2, 2021

I need to revert #2732 and fix it a different way. My bad!

I didn't understand this.

For the other points: got it! It's more clear now.
I'm going to apply those comments at some point between today and Sunday, jfyk.

I'll probably have to bother you to test it again after (if I can't fix the issue I mentioned above).

Thank you for the comments, @bwateratmsft !

@bwateratmsft
Copy link
Collaborator

bwateratmsft commented Mar 2, 2021

I need to revert #2732 and fix it a different way. My bad!

I didn't understand this.

I introduced a bug with #2732 so I gotta fix it or debugging won't work. I have a PR now: #2741

UPDATE: #2741 is now done so all that's needed would be to merge into your fork.

@Kludex
Copy link
Contributor Author

Kludex commented Mar 3, 2021

@bwateratmsft I've applied the changes. Thank you for the update @bwateratmsft :)

Unfortunately I still need to bother you with the testing 😢

Copy link
Collaborator

@bwateratmsft bwateratmsft left a comment

Choose a reason for hiding this comment

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

@bwateratmsft I've applied the changes. Thank you for the update @bwateratmsft :)

Unfortunately I still need to bother you with the testing 😢

Worked like a charm! Didn't encounter any issues on either Window or Mac. Usually Mac means Linux works too, I'm not worried about it.

This is exciting! Thanks @Kludex!

@Kludex
Copy link
Contributor Author

Kludex commented Mar 3, 2021

COOL! 🎉

@bwateratmsft Thanks for all the help! :)

@bwateratmsft bwateratmsft merged commit 02e9ddf into microsoft:main Mar 3, 2021
@microsoft microsoft locked and limited conversation to collaborators Oct 27, 2021
@Kludex Kludex deleted the feat/fastapi-docker branch December 26, 2022 08:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FastAPI Docker Debugger
2 participants