-
-
Notifications
You must be signed in to change notification settings - Fork 37
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 docker image #81
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 echo
s in the shell script and also in the Dockerfile
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 Requestnotebooks
folder, read the Notebooks docs)Goal or purpose of the PR
This PR adds a docker image for the package development.