Add Docker image #80
Add Docker image #80
Conversation
* Edited README.md and streamlit_web.py to include images that are located in README in streamlit website * Replaced with open(README.md) methods with landing_pg() * Change to summarizer * Change pipenv command to only install from lock * Specify with Python in Github Action * Specify python verson in pipenv command * Add id to setup python and remove manual pipenv install * Specify gensim to 3.8.3 * Modify test to clarify what to compare * Remove python 3.6 * Update the recommend version to 3.7 * Fixed error with multiple sidebars * Use landing page if no input found * Show readme in the home page * pushing to check something * pushing again to check if it passes on travis * another push to check if build works :) * amother push change line 41 * editing html like 76 * different img format * fixing image links in readme.md * didnt work because imgs were gone * fixing mdl rules * edited readme file line length (121) * fixed .mdlrc * Changed the streamlit version in Pipfile * Update main.yml * testing * Update main.yml * test 3 * Delete main.yml * Create main.yml * Update main.yml * Update main.yml * Update main.yml * Update README.md * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Update main.yml * Add files via upload * Delete main.yml * Uodated README file * Fixed Readme * Update README.md * Update README.md * Update dockerbuild.yml * Update dockerbuild.yml * Update dockerbuild.yml * Cleaned up Dockerfile * Possible fix * Possible fix pt.2 * Changing OS from 20.04 to latest * Change get-pip.py location * added watchdog to pip install * Move docker script to scripts * Update dockerfile with python image * Add development command * Update action script * Fix reference bug * Use gatorminer as image name * Replace image name with repo name * Replace with repo owner + gatorminer * Convert organization name to lowercase * Change set-env to GITHUB_ENV * Remove redundant repo name Co-authored-by: Nathan Loria <[email protected]> Co-authored-by: zoe <[email protected]> Co-authored-by: Michael Ceccarelli <[email protected]> Co-authored-by: Noor Buchi <[email protected]> Co-authored-by: Bennett Westfall <[email protected]> Co-authored-by: BennyWestsyde <[email protected]>
There was a problem hiding this comment.
Thanks for your work on this feature. I think it's great that this tool will be easily distributed using Docker. I listed some important change requests in my review that should be addressed. Please feel free to ask about my comments if you need clarification or if you're not sure how to approach it. Thanks again!
scripts/docker_run.bat
Outdated
| @@ -0,0 +1,2 @@ | |||
| @echo [+] (Win_OS) Running Docker Container. | |||
| docker container run --name devi -d -p 8501:8501 bennywestsyde/gatorminer && echo "\n\tYou can now view your Streamlit app in your browser.\n\n\tNetwork URL: http://localhost:8501 \n\tExternal URL: http://141.195.4.17:8501" | |||
There was a problem hiding this comment.
Again, this is using @BennyWestsyde docker image, please make sure to change this once the docker image is published to the Allegheny CS Department docker hub organization
scripts/docker_run.sh
Outdated
| #!/bin/bash | ||
| # printf "\n[+] (Mac) Running Docker Container" | ||
| # docker container run --name devi -d -p 8501:8501 bennywestsyde/gatorminer && echo "\n\tYou can now view your Streamlit app in your browser.\n\n\tNetwork URL: http://localhost:8501 \n\tExternal URL: http://141.195.4.17:8501" | ||
| docker run -it -d -p 8080:8501 gatorminer |
There was a problem hiding this comment.
I think that the way that the docker image is being run is inconsistent across scripts.
In the Windows script:
docker container run --name devi -d -p 8501:8501 bennywestsyde/gatorminer
This command is first checking if the image exists locally and if not, it's trying to download and build it from docker hub. This is a great approach but it also means that the docker build command is not really necessary because the command will take care of it. Note: the local machine port here is 8501
On the other hand:
docker run -it -d -p 8080:8501 gatorminer
this command will check if the image exists locally but it will not be able to find it through docker hub if it doesn't exist locally. This is because the organization is not specified. It will still try to build it if it's not present. Also Note: the local machine port here is 8080, which is different than the previous command
Personally, I think that you should not create the scripts and instead simply list the correct docker run command with the organization and repository name in the README file. If you prefer to keep the scripts, I don't think that they should be split up into the docker_build and docker_run since docker run command generally takes care of building the image if it doesn't exist locally.
Please feel free to ask questions on this comment if you need clarifications
There was a problem hiding this comment.
The difference is noted. I apologize for the change in the port number. I was using 8080 while 8501 is occupied with another locally running streamlit instance. It is being changed back to 8501 in the latest commits.
I agree with @noorbuchi that there is probably no need for a script file just for the run command. But I do appreciate the separation of build and run, especially that the build command could be more complex if we are dealing with deployment and version releases. Again, open to suggestions, what do you think @BennyWestsyde?
On another note, is there a specific reason @BennyWestsyde that you chose --name devi as the container name in the command?
|
Update: I tried running the docker image using Is the image on docker hub up to date or is it different from the one in this repository? |
Codecov Report
@@ Coverage Diff @@
## master #80 +/- ##
=======================================
Coverage 92.09% 92.09%
=======================================
Files 6 6
Lines 253 253
=======================================
Hits 233 233
Misses 20 20 |
|
@BennyWestsyde Thanks for you work on this issue and thank you @noorbuchi for the review. I want to point out that this PR is still largely under development and investigation, Many details should be further determined. With that said, I'm responsible for many inconsistencies in the path/command listed across multiple files, and they have been fixed now in the latest commits. The comments left in the As for this feature, I think we need to decide what should go into the newly created standalone repository |
I think this merely a warning message that shouldn't affect the performance of the image/container. However, it's worth looking into whether it could be prevented. While we are on this command: I have noticed that it is extremely slow to import markdown files to the streamlit localhost. I didn't have time t further investigate the reason. Have @noorbuchi and @BennyWestsyde encountered this issue also? |
Sorry for being inactive on this PR for so long. I did not try to import markdown files after starting the docker container but I'm curious about one thing. Does the user need to mount a specific directory to the docker container in order to access the markdown files? |
This GitHub Build Action required that the Repository's owner add two secrets to the secrets tab of the security section. These secrets were never added so the workflow would always fail. So it is being removed in order to make sure that our grade for GitHub Actions CI Build Status is maximized.
@noorbuchi Actually no they don't. As the program currently runs, the user can use all of the template documents that are included in the project, but they can also access their own. If the program wasn't running through the user's internet browser then yes they would have to mount a directory, but since it is run locally on their machine through a browser that, usually, has full disk access already that is unnecessary. |
@enpuyou @noorbuchi I actually just ran the program and uploading the documents was extremely fast for me. I am not sure why it was slow on your end. |
|
Please update your PR/branch with master |
We created the docker image almost entirely. It is able to run in terminal to install all of the dependencies and spacey models. We are still looking to have the container pull from an Allegheny account rather than Bennet's account. We also need to complete documentation in the readme_docker.md file. We have added the documentation in the main readme.md file, but we are open to suggestions of how to improve it. Since this is almost entirely complete, but still a small work-in-progress, we are asking for an overall review as we finish the final steps.