-
Notifications
You must be signed in to change notification settings - Fork 103
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
Adding ml.net centos6 and ubuntu 16.04 container for openmp support #94
Conversation
with openmp support. Fixes issue dotnet#93.
@singlis - Have you considered added these ML specific components into a ML specific image that is built on top of the core images? The primary benefit of this type of layering is it keeps the core image more manageable from a size perspective as well as tracking who depends on the various special components. |
Thanks @MichaelSimons for the suggestion. However OpenMP is not really ML specific. I agree with you completely that any specific requirement should be processed as you are suggesting. |
@singlis - By ML specific I mean it is only being used by the ML product. What other teams are going to be using OpenMP? |
One issue with making a new docker image is the way we are building clang in the centos images. The new docker image would need to duplicate the clang build logic, add the For the |
Thank you everyone for the replies. Eric also brings up a good issue with centos. @MichaelSimons - do you know if OpenMP can be side-loaded after clang is installed? I can try downloading only openmp and see if that can be compiled. |
I confirmed on centos images that I am able to add OpenMP without having re-install Clang. |
@MichaelSimons Let me know how you would like to proceed. I can create an image specifically for ML.net based on the centos 7 and ubuntu 16.04 images -- but where should they be placed? Should they go in this repository? If you feel that this should be part of the dot net images, it maybe worth updating all images to include these changes so thats its a consistent change (its the main reason why I made this a WIP). |
Since nobody is dependent on it outside of ML, my preference would be to put them in a ML specific image. I am alright with including them in this repo. Just put them in a directory underneath the os folder e.g. |
- Reverted changes to centos and ubuntu images
@@ -0,0 +1,26 @@ | |||
FROM mcr.microsoft.com/dotnet-buildtools/prereqs:centos-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.
@MichaelSimons - can you confirm if this is correct?
Also, how do I test this?
And should these dockerfiles be a part of the build?
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.
You can test via .\build.ps1 -DockerfilePath "centos/7*"
Yes these should be part of the build.
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.
You will need to add these to the manifest.json at the root of the repo in order for them to get built.
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 How to modify or create a new image for details on this.
@MichaelSimons I have created ml.net containers for centos7 and ubuntu16.04. Can you review when you have a chance? Thank you very much for your help! |
opensuse is failing due to the opensuse docker container 42.1 is no longer available. I can fix this in my PR if needed. |
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 - just one minor comment.
No need to address the failing opensuse CI with your PR. I will log a separate issue for it.
RUN yum install -y \ | ||
perl-Data-Dumper | ||
|
||
RUN wget http://releases.llvm.org/3.9.1/openmp-3.9.1.src.tar.xz && \ |
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.
A comment here would be valuable IMO just calling out the purpose of the layer - install OpenMP. This was called out in the Ubuntu Dockerfile and that was more obvious why not here as well😸.
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 Michael - since the opensuse issue causes the build to fail, its blocking this PR from going in.
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.
...or maybe I dont have permissions to merge? I will update the PR per your comment.
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.
Not everyone has permission to merge. I will merge once all legs are green except for opensuse.
FYI - the centos build leg is failing it is a network access issue. It is being investigated as time permits. We are busy with release activities today.
The ubuntu build passed and produced the following images:
|
The following centos/7 build issue was resolved.
|
Thanks @MichaelSimons - does it take some time for the docker images to become available? I updated my PR for ML.Net to reference the new docker containers, but it fails to find them. Here is the pull request for reference: |
Your references are not correct, all new images from this repo are now being published to mcr. Note the fully qualified tag names I mentioned earlier - e.g. |
Fixes issue #93.