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

Change docker image to run the simulation on start #61

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

boykovdn
Copy link
Collaborator

@boykovdn boykovdn commented Jun 29, 2023

One less line of commands to run the simulation

Container runs server then checks that server is listening. Then runs p2lab.

The container might have to be changed at some point because the output probably won't be saved anywhere. I'd have to mount some persistent storage and save the simulation results there.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #61 (2cfb3de) into main (f151a98) will decrease coverage by 0.72%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
- Coverage   69.45%   68.73%   -0.72%     
==========================================
  Files          15       15              
  Lines         478      483       +5     
==========================================
  Hits          332      332              
- Misses        146      151       +5     
Impacted Files Coverage Δ
src/p2lab/__main__.py 64.61% <50.00%> (-0.39%) ⬇️

... and 1 file with indirect coverage changes

@phinate
Copy link
Collaborator

phinate commented Jun 30, 2023

Thanks for this @boykovdn -- looks good. I'm thinking we could probably move in the direction of having this read in from a config file, as this will run the simulations with default arguments everytime. We can even use that to define the directory that docker mounts to print out results I would think!

I can happily take a look at this and build on this PR at some point, but if you have time and want to I can let you lead that :)

@boykovdn
Copy link
Collaborator Author

Nice idea, I'll maybe have a look and give it a shot soon!

@phinate
Copy link
Collaborator

phinate commented Jun 30, 2023

Great! I'll leave this PR for the time being then -- thanks :)

Example config.yaml added to the root of the repo.

Dockerfile now contains a new environment variable which tells the container where it should look for the config file.

Additional dependency on pyyaml to parse the config file.

__main__.py now parses the arguments in from the config file rather than from inputs.

README.md updated with the command that runs the container with a custom config file. By default the docker image will be build with the config.yaml file from the repository. This can be overridden by mounting a config from the local filesystem using the -v flag as shown in the README.md example.
@boykovdn
Copy link
Collaborator Author

boykovdn commented Jul 1, 2023

Changed the main.py to run with a config file. I don't know if this is the best way to parametrise the container, and I couldn't test the full building + running because the container is built from the main branch directly 😅

I did try overriding the source in the container with the one from my local filesystem and it seemed to work, so hopefully it's fine!

@phinate
Copy link
Collaborator

phinate commented Jul 3, 2023

@boykovdn ok this is great!

regarding the fact you clone on the fly, I think we can probably change it to just copy all the relevant files from the Dockerfile directory into the container. Then you can test it properly :)

Also, an accidental consequence of using the configfile in the way you do is that you erase the command-line arguments! So maybe you can put that back in, and add a new arg called config that is the path to the config file, which then reads all the relevant args. Then, a possible next step is that you can specify that as a docker build argument (i found this random page when googling), which makes sure to copy the config path into the container, and you can then run p2lab --config=$CONFIG_PATH or however that's fed in!

After these two changes, then we're definitely good to merge :)

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.

3 participants