-
Notifications
You must be signed in to change notification settings - Fork 435
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: initial Dockerfile support #2046
Conversation
Can one of the admins verify this patch? |
@ashwinvaidya17, can you review this PR please? |
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.
Thanks for the PR. I have one comment regarding the placement of this file. The rest looks good.
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 contribution @QSmally!
It looks generally OK but one only concern of mine is that the majority of use-cases of model training will run on GPUs. Current PR covers only Python3.9 + CPU.
@samet-akcay What do you think? Will we start from CPU and extend GPU support later?
Ty @goodsong81, @ashwinvaidya17, @yunchu, I implemented build-arg support for However, I lack the installation knowledge and machine to get the GPU dependencies, so I'd like assistance with that |
Thanks for the contribution @QSmally. In this case, GPU support could be addressed by a follow-up PR by the team later. |
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.
Thanks
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.
LGTM. @QSmally, Thank you for your contributions!
@ashwinvaidya17 @yunchu Could you help pass the Dockerfile style check? I think the CI is regarding the warning as error. |
@goodsong81, we can update linter setting for dockerfile but I think it's better to resolve all warnings. |
I rarely encounter a Dockerfile with pinned apt versions, so I'm not sure how (and if) we should update that, @yunchu/@goodsong81 |
@QSmally
|
@QSmally I'm not sure but https://github.com/openvinotoolkit/training_extensions/blob/develop/.ci/Dockerfile this one could be the example for you. |
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 contribution!
Summary
This PR implements a basic Dockerfile in contrast to using Python virtual environment (as I personally had some dependency problems with it). It's basic, as in, it installs the CPU dependencies and leaves CUDA out of the mix. Maybe with build-args you can change some things but I don't have a Nvidia setup to test it.
How to test
Checklist
License
Feel free to contact the maintainers if that's a concern.