Add docker image #81
Add docker image #81
Conversation
|
@lilianabs looks neat, nice work! I will test it out and share my comments here. |
|
Could you pls post a screen shot of the docker container running and giving you a prompt as per the code - does not have to be exact or working, just want to see what it looks like on your end. It's easy to take a screenshot and drag it into the comment box and it turns it into a image url to link to |
There was a problem hiding this comment.
Well done, good work!
Overall looks good, pending some good testing on my part.
Also shared comments and suggestions on existing code.
I will do another pass and share some more comments, in the meanwhile please try to implement/improve what you can - based on the comments shared.
docker-image/Dockerfile
Outdated
| @@ -0,0 +1,17 @@ | |||
| FROM python:3.7 | |||
There was a problem hiding this comment.
See if you can make this python version configurable that you can pass it via the shell script or environment variable.
If you look into the docker shell scripts in any of these examples here, you can see how it's done.
There was a problem hiding this comment.
Thank you for pointing this out. I've updated the Dockerfile to pass the argument via the shell script.
| @@ -0,0 +1,17 @@ | |||
| FROM python:3.7 | |||
|
|
|||
There was a problem hiding this comment.
Try to get familiar with hadolint and apply it to your Dockerfile to see what suggestions it gives you in terms of improving the code, here's a post to learn more about it
There was a problem hiding this comment.
I've used hadolint to format the Dockerfile accordingly :)
docker-image/Dockerfile
Outdated
|
|
||
| RUN apt-get install -y git | ||
|
|
||
| RUN git clone https://github.com/neomatrix369/nlp_profiler.git |
There was a problem hiding this comment.
Instead of doing this try to see if you can actually use the local folder and the code downloaded in it - there is an advantage and purpose for doing it.
See how it has been done here.
There was a problem hiding this comment.
Thank you for the suggestion Mani. I've updated the file so it uses the local folder.
| echo "Mounted volume: ${MOUNT_VOLUME}" | ||
|
|
||
| docker build -t ${DOCKER_IMAGE_NAME} . | ||
|
|
There was a problem hiding this comment.
It's a good practice to show this line executing with all its parameters, how would you do that in bash?
Also look at one of the examples from the resources shared above
| @@ -0,0 +1,20 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Try to get familiar with a bash shell linter to see what comments it has about your shell script - search for shellcheck, install it and then use it on this shell script.
Use the comments from shellcheck to improve the code if any comments are given
|
|
||
| echo "~~~ Running nlp_profiler in a docker container" | ||
| echo "Mounted volume: ${MOUNT_VOLUME}" | ||
|
|
There was a problem hiding this comment.
Its a good practise to echo the steps that are executed at the different lines in the code - this way when one reads the console they can tell what stage of the shellscript it is at - look at examples of my own code to find out how we have been doing it, the clue is look for echos in the shell script and also in the Dockerfile
Thank you for reviewing the PR Mani! Here is a screenshot: 
|
To be able to merge a pull request, there are a few checks:
DO NOT DELETE THE BELOW TEXT FROM THE BODY OF THE PULL REQUEST. ADD YOUR DESCRIPTION(S) FROM THE POINT IT SAYS "Goal or purpose of the PR". USE THIS TEMPLATE AS IS.
USE THE CHECKLIST TO SHOW PROGRESS AND A REMINDER FOR YOURSELF.
Checklist
Please check the options that you have completed and strike-out the options that do not apply via this pull request:
./test-coverage.sh "tests slow-tests") - this will also be visible via the Code coverage report and CI/CD task on the Pull Requestnotebooksfolder, read the Notebooks docs)Goal or purpose of the PR
This PR adds a docker image for the package development.