-
Notifications
You must be signed in to change notification settings - Fork 44
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
[ODSC-48942]opctl create/publish error on M1 and M2 #381
Conversation
|
with tempfile.TemporaryDirectory() as td: | ||
process = subprocess.Popen( | ||
["conda", "env", "export", "--prefix", pack_folder_path], | ||
stdout=subprocess.PIPE, | ||
stderr=subprocess.PIPE, | ||
) | ||
stdout, stderr = process.communicate() | ||
if stderr: | ||
|
||
if process.returncode and stderr: | ||
print(stderr) |
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.
Let's use logger for debug purpose?
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 remember we could only use print() to show the msgs when running this code in container?
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.
Yep yep, you are right.
ads/opctl/docker/Dockerfile_arm.job
Outdated
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.
NIT: Maybe Dockerfile.arm.job
?
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.
good suggestion! will change.
ads/opctl/docker/Dockerfile_arm.job
Outdated
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.
Just curious, the ARM docker file looks much bigger than x86 one? Was it copied from the jupyter repo?
This file I guess needs to be cleaned up. The Jobs file was much lighter in comparison with the jupyter one. We don't need to install jlab related libraries for example, and YAM i think needs to be installed differently.
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.
updated with Liuda's PR
ads/opctl/docker/base-env-arm.yaml
Outdated
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.
Is this base yaml different from the one that we already have?
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.
updated with Liuda's PR
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.
We probably don't need this certificates if we get rid of the libraries that relay on 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.
removed
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.
Do we really need this file?
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.
updated with Liuda's PR
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.
We probably don't need this file, if we don't want to run the JLab server?
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.
updated with Liuda's PR
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.
We probably can get rid of this file?
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.
updated with Liuda's PR
|
|
|
with tempfile.TemporaryDirectory() as td: | ||
process = subprocess.Popen( | ||
["conda", "env", "export", "--prefix", pack_folder_path], | ||
stdout=subprocess.PIPE, | ||
stderr=subprocess.PIPE, | ||
) | ||
stdout, stderr = process.communicate() | ||
if stderr: | ||
|
||
if process.returncode and stderr: | ||
print(stderr) |
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.
Yep yep, you are right.
@@ -161,7 +162,14 @@ def build_image( | |||
) | |||
proc = _build_custom_operator_image(gpu, source_folder, dst_image) | |||
else: | |||
image, dockerfile, target = _get_image_name_dockerfile_target(image_type, gpu) | |||
# https://stackoverflow.com/questions/66842004/get-the-processor-type-using-python-for-apple-m1-processor-gives-me-an-intel-pro | |||
import cpuinfo |
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 might be better to create a separate function - _get_architecture()
? We probably will reuse this method in different other places.
|
|
📌 Cov diff with main: No success to gather report. 😿 📌 Overall coverage: No success to gather report. 😿 |
1 similar comment
📌 Cov diff with main: No success to gather report. 😿 📌 Overall coverage: No success to gather report. 😿 |
ads/opctl/docker/Dockerfile_arm.job
Outdated
WORKDIR /home/datascience | ||
|
||
COPY base-env-arm.yaml /opt/base-env.yaml | ||
COPY jupyter-arm.yaml /opt/jupyter.yaml |
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 think we need only 1 yaml here. In Dockerfile.job - we have one. And in jobs Dockerfile.arm (https://bitbucket.oci.oraclecorp.com/projects/ODSC/repos/odsc_jobs/browse/jobs/Dockerfile.arm) also 1 yaml used - https://bitbucket.oci.oraclecorp.com/projects/ODSC/repos/odsc_jobs/browse/jobs/Dockerfile.arm#89-90
ads/opctl/docker/Dockerfile_arm.job
Outdated
ARG PIP_INDEX_URL | ||
|
||
############# Setup Conda environment tools ########################### | ||
ARG RAND=1 |
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.
We creating special directory for operators, setting RUN_WORKING_DIR etc., not sue if this a requirement for the image, but Dockerfile.job has this lines - https://github.com/oracle/accelerated-data-science/blob/main/ads/opctl/docker/Dockerfile.job#L108C1-L108C17
And we need to finish file with USER datascience. I would suggest to copy lines from link above to this Dockerfile also.
@@ -0,0 +1,8 @@ | |||
[ol7-yum] | |||
name=ol7-yum | |||
baseurl=https://artifactory.oci.oraclecorp.com/ol7-yum/ |
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.
Is Artifactory in reach for users? This is not a public repository
@@ -123,6 +123,7 @@ opctl = [ | |||
"nbconvert", | |||
"nbformat", | |||
"oci-cli", | |||
"py-cpuinfo", |
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.
New library will need an input into https://github.com/oracle/accelerated-data-science/blob/main/THIRD_PARTY_LICENSES.txt
ads/opctl/docker/Dockerfile_arm.job
Outdated
WORKDIR /home/datascience | ||
|
||
# Note in order to run sudo commands as a non root user, you must specify --credential yes if using qemu static to build the image | ||
RUN wget -nv https://artifactory.oci.oraclecorp.com/odsc-dev-generic-local/datascience/Miniconda3-${MINICONDA_VER}-$(uname)-$(uname -m).sh \ |
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.
We can't use here link to artifactory, this is internal resource.
Use repo.anaconda link for miniconda - https://bitbucket.oci.oraclecorp.com/projects/ODSC/repos/odsc_jobs/browse/jobs/Dockerfile.arm#73
And gihub for miniforge, like described here - https://github.com/conda-forge/miniforge#mambaforge.
|
|
https://jira.oci.oraclecorp.com/browse/ODSC-48942
Testing Step
build the image
ads opctl build-image job-local
conda create
ads opctl conda create -n testing -f ~/Downloads/environment.yaml
conda publish
ads opctl conda publish -s /Users/z7ye/conda/testing_v1 -a security_token
Evidence