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

Docker setup #502

Merged
merged 10 commits into from
Oct 30, 2018
Merged

Docker setup #502

merged 10 commits into from
Oct 30, 2018

Conversation

wtgee
Copy link
Member

@wtgee wtgee commented Mar 4, 2018

Adding initial Dockerfile and updated instructions.

We could design the container better so it is not such a monolithic service but this gets at least the initial file there. Also needs to be reduced in size.

Closes #27

@wtgee wtgee requested a review from a team March 4, 2018 04:50
@codecov
Copy link

codecov bot commented Mar 4, 2018

Codecov Report

Merging #502 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #502      +/-   ##
===========================================
- Coverage    77.71%   77.69%   -0.02%     
===========================================
  Files           61       61              
  Lines         5080     5080              
  Branches       697      697              
===========================================
- Hits          3948     3947       -1     
- Misses         922      923       +1     
  Partials       210      210
Impacted Files Coverage Δ
pocs/serial_handlers/protocol_arduinosimulator.py 73.84% <0%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 268110a...25656f6. Read the comment docs.

@jamessynge
Copy link
Contributor

Are you seeking a review on this still? If so, I suspect @mimming would be your best bet.

@wtgee
Copy link
Member Author

wtgee commented May 26, 2018

Are you seeking a review on this still? If so, I suspect @mimming would be your best bet.

I might change this still so am leaving it open. I'm not sure that starting up the jupyterlab is the best default option.

wtgee added 5 commits July 18, 2018 10:25
* Reducing size
* Starting CMD correctly
* Skipping git for zip
* Adding env variable for solve-field
* Adding git back to included
* Start up a bash shell for CMD
	Note: It's expected this is used as a base for other images and
	so the CMD will be overriden.

#### Let Docker use gcloud

Docker needs to be able to use your `gcloud` login to pull the PANOPTES images. There
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think that the "advanced-authentication" is still necessary? The page now recommends just using this command:

gcloud auth configure-docker

After that (assuming I'd not previously installed docker-credential-gcr, which it doesn't appear that I did according to gcloud components list), running this command worked:

docker pull gcr.io/panoptes-survey/pocs:latest

Copy link
Member Author

Choose a reason for hiding this comment

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

This could have changed, I'll check it out.

docker run -it -p 9000:9000 --name pocs gcr.io/panoptes-survey/pocs
```

The POCS image will automatically start [Jupyter Lab](https://jupyter.org/) running
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be aspirational, but not currently true. The command is just /bin/bash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it worked perfectly fine but I removed it. If we leave the CMD as /bin/bash then you can launch whatever you want when you start the instance so it's a bit more generic.

I'll fix the README, thanks.

@jamessynge
Copy link
Contributor

Looks like ffmpeg needs to be added to the deps used when creating the image.
And really we should probably look at breaking this up into multiple layers.

@jamessynge
Copy link
Contributor

And now I see that the same thing (ffmpeg being missing) happens in travis-ci.

@wtgee
Copy link
Member Author

wtgee commented Sep 1, 2018

Looks like ffmpeg needs to be added to the deps used when creating the image.

Will do.

And really we should probably look at breaking this up into multiple layers.

It could certainly be optimized some more. Also, this is acting as a lower layer for the piaa and other docker containers that are in there.

One thing to keep in mind is that my update of this Docker changed the scope a bit although it might not matter. I was originally making this for the running of a unit and/or jupyter-lab instance connected to the unit. The only way I'm currently using it as a base to build an image for compute instances for doing algorithm work. However, when I start those instances I want solve-field, sextractor, etc. without having to install.

Layers will help but we should also figure out exactly how we want to use the containers.

And now I see that the same thing (ffmpeg being missing) happens in travis-ci.

I see that occasionally (it's always there) and keep forgetting to do anything about it. Should probably just suppress warnings as we don't actually need to make the movies on travis.

@wtgee
Copy link
Member Author

wtgee commented Oct 11, 2018

@jamessynge I think this could be reasonably merged. Could feasibly remove the changes to the README and just leave the Dockerfile. Thoughts?

@jamessynge
Copy link
Contributor

How about creating a docker directory, and putting the Dockerfile and relevant portions of the README into that directory. Then I think this would be good to go.

Copy link
Contributor

@jamessynge jamessynge left a comment

Choose a reason for hiding this comment

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

Approving on the assumption that Wilfred agrees with the proposal to move Dockerfile and the relevant portion of the README to a docker directory.

@jamessynge
Copy link
Contributor

I want to build on this, so rather than wait for your updates, I'm going to merge as is, then will make changes I suggested.

@jamessynge jamessynge merged commit f1e67c2 into panoptes:develop Oct 30, 2018
@wtgee wtgee deleted the docker-setup-27 branch October 30, 2018 20:42
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.

2 participants