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

[Feature]: Extends Pull Request #152 Regarding Docker File and Configuration #172

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sanketshivale
Copy link

  • Added and Refactored Dockerfile to utilize Docker's layer caching feature for faster builds .
  • Updated README.md for installation and setup of Docker.
  • Optimized Dockerfile by copying requirements.txt first, followed by the rest of the project files later so that we can use caching on build.
  • Updated integration.py for get_monitor fixes in Docker.

@Udayraj123
Copy link
Owner

Great to see quick action! Please add a screenshot or short video recording of how it's working on the machine if possible :)

@Udayraj123 Udayraj123 changed the title [Feature]: Extends Pull Request #152 Regrading Docker File and Configuration [Feature]: Extends Pull Request #152 Regarding Docker File and Configuration Mar 5, 2024
@sanketshivale
Copy link
Author

Yes for sure !! Here is the screen recording of the build and run process of docker of OMRChecker.

simplescreenrecorder-2024-03-06_01.06.23.mp4

# If running in a container, make a fake monitor
monitor_window = (
Monitor(0, 0, 1000, 1000, 100, 100, "FakeMonitor", False)
if os.environ.get("OMR_CHECKER_CONTAINER")
Copy link
Owner

Choose a reason for hiding this comment

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

I think we shall also disable the call for imshow using this code change -

def show(name, origin, pause=1, resize=False, reset_pos=None, config=None):
        if(os.environ.get("OMR_CHECKER_CONTAINER")):
            return

Copy link
Author

Choose a reason for hiding this comment

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

I required some assistance with these. As of now, I understand that we aim to refrain from displaying image metrics in DOCKER CONTAINER. If I am correct in this understanding, then with my current knowledge, I attempted to address the issue on my end. Please let me know if any clarification is needed.

Copy link
Owner

Choose a reason for hiding this comment

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

You can test it once by setting show_image_level to 5 in one of the samples, the code should still finish without any errors. If that works fine, above change should suffice.

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Udayraj123
Copy link
Owner

Credits to @youainti for creating #152 as the foundation

@youainti
Copy link

Thanks for getting this across the finish line. The last 6 months have been very busy for me.

@Udayraj123
Copy link
Owner

I guess I jinxed it 😄. Still pending on review comments and I have one more suggestion now -
How about introducing a devcontainer.json file for working in Github codespaces (inspired from OpenMCR)

@sanketshivale
Copy link
Author

sanketshivale commented May 6, 2024

@Udayraj123 video for the changes

Now, the next step in dealing with the devcontainer is to review the pull request. If there are any necessary changes, please share them for improvement.

simplescreenrecorder-2024-05-06_23.38.56.mp4

@Udayraj123
Copy link
Owner

Great! I'll check this over the weekend and try to get your PR merged!

cd OMRChecker/
```

2. Build the Docker container image (Initial build may take up to 2 minutes depending on your network connection):
Copy link
Owner

Choose a reason for hiding this comment

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

After merging the dev branch, we will also consider publishing a docker image on dockerhub

Udayraj123
Udayraj123 previously approved these changes May 9, 2024
@Udayraj123 Udayraj123 dismissed their stale review May 10, 2024 21:11

Changing mind to create a dedicated readme for docker setup


### Running OMRChecker in Docker

Once Docker is installed, you can run OMRChecker within a Docker container by following these steps:
Copy link
Owner

Choose a reason for hiding this comment

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

Let's move this section into a dedicated file.

Suggested change
Once Docker is installed, you can run OMRChecker within a Docker container by following these steps:
For running OMRChecker using docker, please check the [readme-docker.md](https://github.com/Udayraj123/OMRChecker/tree/master/readme-docker.md)

Move this section details into a top level file readme-docker.md

@Udayraj123
Copy link
Owner

Almost forgot the docker compose file from this PR. I think we should add that too!

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.

None yet

3 participants