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

Container not re-used on code change #6

Open
ptrhck opened this issue Jun 3, 2022 · 17 comments
Open

Container not re-used on code change #6

ptrhck opened this issue Jun 3, 2022 · 17 comments

Comments

@ptrhck
Copy link

ptrhck commented Jun 3, 2022

I have been trying this example and wanted to check whether hot code reloading also works when running the Docker container with pants. What I noticed is, that on every code change a new container is run. For a lot of code changes, this would spin up a lot of containers. Am I missing something to be configured?

@kaos
Copy link
Member

kaos commented Jun 3, 2022

It will build a new image for every change, yes. When running, you may want to pass the '--rm' flag to docker so terminated containers are cleaned up automatically.

@ptrhck
Copy link
Author

ptrhck commented Jun 3, 2022

I am just wondering what a good debug strategy would be as I would need to rerun the debugger to attach again after every code change. Right?

Can IDEs understand PEX files for debugging purposes anyway?

@stuhood
Copy link
Member

stuhood commented Jun 3, 2022

Can IDEs understand PEX files for debugging purposes anyway?

See https://www.pantsbuild.org/docs/python-run-goal#debugging for info on how to do this outside of docker... inside of docker, it should be largely the same, but with exposed ports.

@kaos
Copy link
Member

kaos commented Jun 4, 2022

@ptrhck

I am just wondering what a good debug strategy would be as I would need to rerun the debugger to attach again after every code change.

What is your setup here? I mean, what does the Docker container give you that running the Pex/code directly doesn't when debugging?

@ptrhck
Copy link
Author

ptrhck commented Jun 7, 2022

I am used to running my applications in a isolated environments, i.e. Docker containers so that I can replicate my production environment as much as possible and that seems a best practice in the container world. One other setup might be to use https://github.com/localstack/localstack for running the applications, even though pants build/publish is used instead of pants run.

What is the intended use case for the pants run command for a docker target?

@kaos
Copy link
Member

kaos commented Jun 7, 2022

Got it. +1 for having a production like environment for testing, however depending on the change/build/test cycle time for such a setup, being able to also develop under a less strict environment may be worth considering. (I don't have experience developing for cloud services, so that remark may be off by some degree..)

What is the intended use case for the pants run command for a docker target?

The run support for the docker_image target is a convenience so you can easily build and run any image in your project, so you don't have to figure out what the built image name is to run. Nothing more than that.

For tighter integration with the Pants-world, and the use case you describe, I think may be covered by pantsbuild/pants#13682 which you've discovered earlier ;)

@ptrhck
Copy link
Author

ptrhck commented Jun 13, 2022

targe

So I tried the --rm flag, which worked so far. However, when I also specify a container name ./pants run --docker-run-args="-p 9999:9999 --name django_demo --rm" django:django_dev_image -- runserver 0.0.0.0:9999 --noreload it seems not to remove the container in time before creating a new one:

docker: Error response from daemon: Conflict. The container name "/django_test" is already in use by container "52b32125058a1ba1a752633106dadf7b2bf0247d13f791cced7365554229f36d". You have to remove (or rename) that container to be able to reuse that name.

Any idea? I would like to investigate how to attach a VSCode debugger to the container and I might need to identify the container and thought the name seems a good option.

@kaos
Copy link
Member

kaos commented Jun 13, 2022

Ah, so docker doesn't remove the old container quick enough. Another option, going down this route, could be to write out the container id to a file mounted from the host, and monitor that file for changes should allow the connection with VSCode to be kept up-to-date, hopefully (no idea how that connection works)

@ptrhck
Copy link
Author

ptrhck commented Jun 14, 2022

That is a good hint. Another option could be using the label and filter for it.

In general, the debug setup for VSCode seems to be different than with IntelliJ, where you run a debug server from the code. For VSCode, it looks like it runs the app and hooks into the process. That is why I was asking for debugging PEX in general as I am not sure VSCode can be configured to understand this. There seems to be options where we might be able to specify that pants should run the container. I will try to investigate this and any hints are very appreciated.

In theory, is it possible to not use pants code reloading but mount the source code to the container and use it that way for within the PEX File?

@kaos
Copy link
Member

kaos commented Jun 14, 2022

In theory, is it possible to not use pants code reloading but mount the source code to the container and use it that way for within the PEX File?

I think you'd have to leave the Pants world entirely for that, as it is.

@ptrhck
Copy link
Author

ptrhck commented Jun 23, 2022

In theory, is it possible to not use pants code reloading but mount the source code to the container and use it that way for within the PEX File?

I think you'd have to leave the Pants world entirely for that, as it is.

It should work for the AWS Lambda case though as here the PEX will be unzipped: ./pants run --docker-run-args="-v ./lambda_example.py:/app/lambda_example.py" project:my_image. Right?

So there is no current possibility to achieve this in a similiar way for just a normal PEX file?

As a new feature I could think of something like a --rebuild flag. The container image will be build if it has not been created yet. But for all subsequent file changes, the whole image will not be rebuild but just the PEX file mounted to the container. If you have changed the Dockerfile you could add this flag to your ./pants run command. This would also resolve the above problem where the container is not being removed on time. Thoughts?

@kaos
Copy link
Member

kaos commented Jun 23, 2022

Ah, yes. That is interesting. So the image is used more as a side-effect/or means for debugging, rather than build me an image, and then I want to run it.

IIUC we'd want to be able to say "continuously build this target, and volume mount it into a container based on this image running command x". Effectively setting up one container with one command with the built artifact volume mounted into the container, and replace the artifact on any source changes while keeping the container running.

I think that's doable.

@kaos
Copy link
Member

kaos commented Jun 23, 2022

@thejcannon does any of the above fit with the --debug-adapter (or even --run-adapter) thingy.. ? (if we'd implement said adapter for such a purpose)

@thejcannon
Copy link
Member

I think so? I'm only partially grokking the discussion, but if you're trying to VS Code debug some code, the test goal's --debug-adapter option allows you to do so (with a run goal implementation coming soon(-ish)).

I owe Pants the docs for this, but I'm waiting for run's flavor before I do.

@ptrhck
Copy link
Author

ptrhck commented Jun 24, 2022

@thejcannon I had a look and the --debug-adapter and that is a great feature for debugging purposes for sure! I am not sure what @kaos is referring to regarding this issue to be honest.

@kaos What I have missed with the mentioned AWS Lambda example: Yes, the hot code reloading should work that way, but of course any third party or internal lib changes will not be recognized.

Ah, yes. That is interesting. So the image is used more as a side-effect/or means for debugging, rather than build me an image, and then I want to run it.

IIUC we'd want to be able to say "continuously build this target, and volume mount it into a container based on this image running command x". Effectively setting up one container with one command with the built artifact volume mounted into the container, and replace the artifact on any source changes while keeping the container running.

I think that's doable.

Yes, that is absolutely right. The image is, in the best case, the production image and I want to run my code with pants in that container for developing/debugging the application. With the --debug-adapter for run, this would allow me to attach to that container for debugging.

With the current implementation of pants run Dockerfile and restartable=True, the developer experience is, compared to a normal mount of source code into a container, not that great. Because, this will rebuild the image all the time which might not be necessary. The problem of the container not being removed on time aside.

So yes, I could think of the following workflow for pants run Dockerfile for greater iterations in development and I guess this would be more the "Docker-way":

  • Build the image if not yet available
  • Mount the build artifact, e.g. PEX-file to the container
  • On file change, rebuild that artifact and due to the container mount it will be available in that environment without recreating the container or rebuilding the image
  • If you have changed the Dockerfile, a --build flag could to added to the pants run command, similiar to docker-compose up --build serviceA

What are your thoughts? Should I open a feature request?

@kaos
Copy link
Member

kaos commented Jun 24, 2022

That aligns with my current understanding.

My reference to the debug adapter here would be more of an implementation detail (and possibly UX) not functionality.

I think we can work with this issue as the feature request, but feel free to write a new one if that makes better sense to you :)

@kaos
Copy link
Member

kaos commented Jun 24, 2022

Oh, just realized this issue is not in the pants repo, so yes a feature request over in pantsbuild/pants would be great, thank you.

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

No branches or pull requests

4 participants