-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add VS Code debug configuration #48
Conversation
Adds a docker-compose.debug.yml file based on the existing docker-compose.yml file used in dev, which overrides the default command in order to install debugpy and wait for a debugger to attach via port 6789 prior to running the flask dev server process.
Adds configuration for debugging the `save_routes.py` script using the flask-dev Docker image. This configuration can be used as prior art for adding other script debug configuration in the future, or adjusted to accept a user-specified command to run rather than creating vscode tasks for each script. Note: a prompt was added to allow M1/M2 Mac users to select a working platform; this will ideally be removed in the future as support for ARM64 wheels is added to installed packages, but is necessary for now to successfully build the Docker image.
"inputs": [ | ||
{ | ||
"id": "dockerBuildm1", | ||
"type": "pickString", | ||
"description": "Select '--platform linux/amd64' to build for Macs with an ARM CPU", | ||
"default": "", | ||
"options": [ | ||
"--platform linux/amd64", | ||
"" | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love having to add this option, but hopefully as mentioned in the commit message it won't be necessary in the long term. Another option would be to try to get the architecture through some environment variable and set this automatically, but that seemed like it might be more work than it was worth at this point.
"docker-build" | ||
], | ||
"python": { | ||
"args": ["--s3", "--timetables", "--scheduled-stats", "--agency=trimet"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worthwhile to shift this towards a user-specified command rather than this hardcoded option opportunistically as other tickets are worked on that involve debugging other scripts. I'm open to giving that a go now though if that seems better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this format for now. I also like the idea of a user-specified command but I think that can be a v2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there! Looking good so far. Just had a couple quick questions. I haven't gotten it working yet on my end (the debugger launches but breakpoints don't seem to hit), so probably user error.
{ | ||
"name": "Docker: flask-dev - save_routes.py", | ||
"type": "docker", | ||
"request": "launch", | ||
"preLaunchTask": "docker-run: save_routes.py", | ||
"python": { | ||
"pathMappings": [ | ||
{ | ||
"localRoot": "${workspaceFolder}/backend", | ||
"remoteRoot": "/app/backend" | ||
} | ||
], | ||
"projectType": "general" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the purpose of this Docker launch config? I would have assumed the Python: Attach
is all we would have needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! The Python: Attach
launch config I added is, at this point, just used in conjunction with the docker-compose file added in this PR to be able to debug the flask app. This block plus the code in tasks.json
is used to build and run a flask-dev
container that calls the save_routes.py
script. (In one of my comments above, I mentioned we might want to expand this to accept user input for the specific cli script to run, but I started with save_routes
, since that's what I was working on in #42.) This config makes it possible to launch a container that will call that script in one click from the debug and run sidebar.
I think it would be possible to use the Python: Attach
config if we wanted to have devs start a shell via ./docker-shell.sh
, then pip install debugpy
, then add something like the block in this SO post to their script, and then call the script. And we'd want to open the debug port in the Dockerfile
as well.
In writing this up, and thinking about your comment below, it feels like there should be some solution where we'd have an entrypoint script for starting up an image that takes care of installing debugpy
and calling it with the desired port to listen on, and then being able to pass in the command to run. Basically, it would be great if we could docker compose up
with some reference to an optional entrypoint script and also optionally pass in the desired command (either flask run
, or a specified script). I have not played around with that though, so I'm not sure how straightforward it is to build in that level of flexibility, without overcomplicating the process for someone who just wants to docker compose up
the dev stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For installing debugpy
I'm wondering if we can just make that part of the default Dockerfile. Does it add a lot of packages/dependencies? If it's pretty small, it might be worth just having it in the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Just getting back to this after having been out of town (and preparing to leave town again 😬 ). So I'm not sure how much the addition of debugpy
to the flask-dev
image buys us, in terms of file / code reduction. We'd still need some way to override the default command when starting up that container (and we'd need port 6789
in the compose service definition - but that's a fairly inconsequential addition).
Having messed around with this a little more, I think we could at least reduce the contents of the docker-compose.debug.yml
file, if we wanted to go the route of having folks run docker compose up
with multiple compose files (kind of like the example I added to the docs for including the docker-compose.overrides.yml
file, but the debug file would be added too). The debug compose file would just extend the definition from the base docker-compose.yml
file. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, after trying this out a little more, I think it would work to have the following in the docker-compose.debug.yml
file:
version: "3.7"
services:
flask-dev:
command: ["sh", "-c", "pip install debugpy -t /tmp && python /tmp/debugpy --wait-for-client --listen 0.0.0.0:6789 -m flask run --no-debugger --no-reload --host 0.0.0.0 --port 5000"]
ports:
- 6789:6789
And then the debug compose config could be started up with something like docker compose -f "docker-compose.yml" -f "docker-compose.debug.yml" -f "docker-compose.override.yml" up --build
That would at least reduce the amount of duplicative code between the base and the debug compose files, and keep the vs code debug option opt-in rather than opt-out. I'm definitely open to other thoughts or suggestions though!
dockerfile: Dockerfile | ||
target: flask-dev | ||
context: . | ||
command: ["sh", "-c", "pip install debugpy -t /tmp && python /tmp/debugpy --wait-for-client --listen 0.0.0.0:6789 -m flask run --no-debugger --no-reload --host 0.0.0.0 --port 5000"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both this and the collector is starting to build up a lot of docker-compose files (I'm guilty of adding to this on the collector side), I wonder if since this compose file seems to only differ in overriding the Dockerfile CMD
step for the metrics-flask-dev
image, if we can make the build command (seen here
opentransit-metrics/Dockerfile
Line 21 in 75ccd2b
CMD ["flask", "run", "--host", "0.0.0.0"] |
ARG
that can be passed in from the VSCode Launch config.
I'd need to mess around with this a bit. Potentially a future enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know what you mean about having a lot of docker-compose files! :) I'm not sure how much config is possible to pass in to a docker-compose invocation from vscode. It's definitely a little more straightforward to customize starting up a single docker image from vscode. Or at least most of the vs code debug docs for docker compose focused on having some sort of debug file like this.
Thanks for taking a look @liamphmurphy! That's unfortunate that this config hasn't worked for hitting any specified breakpoints for you yet 🤔 Were there any errors that popped up when you tried it? I saw some messages that I believe were related to the |
…onfig Updates the `docker-compose.debug.yml` config to define only settings that should override or extend the base `docker-compose.yml` config.
Ok, @sidetrackedmind just pushed those changes we talked about! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing the changes @nkanderson! Both Python: Attach
(after running docker compose -f docker-compose.yml -f docker-compose.debug.yml up
) and Docker: flask-dev - save_routes.py
debug configurations work for me. As we discussed during the meeting, I think these configurations are helpful to have as they exist. If people end up using them a lot we may want to expand the configurations but that's a future issue/request. Thanks again for all the hard work 🙌
Proposed changes
Adds VS Code configuration and docs for debugging the Flask app and Python command line scripts.