Fix for dmidecode + support mounting filepaths in docker #344
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
| @@ -3,4 +3,4 @@ | |||
| if [[ ${MLC_SUDO_USER} == "yes" ]]; then | |||
| ${MLC_SUDO} dmidecode -t memory > ${MLC_MEMINFO_FILE} | |||
| fi | |||
| test $? -eq 0 || return $? | |||
| test $? -eq 0 || echo "Warning: Memory info is not recorded" | |||
There was a problem hiding this comment.
dmidecode exits with error no device error. Maybe due to permission error with dmidecode inside docker containers. Would it be okay if we just print warnings instead of exiting.
anandhu-eng
changed the title
automation/script/docker_utils.py
Outdated
| @@ -78,10 +78,19 @@ def process_mounts(mounts, env, docker_settings, f_run_cmd): | |||
|
|
|||
| # Update container environment string and mappings | |||
| if host_env_key: | |||
| container_env_string += f" --env.{host_env_key}={container_env_key} " | |||
| # We are modifying the input mapping variables wrt to the new container env variable. Is explicit passing of env variable necessary? | |||
There was a problem hiding this comment.
What is the run command is directly using the env variable?
automation/script/docker_utils.py
Outdated
| @@ -392,7 +392,7 @@ def get_docker_default(key): | |||
|
|
|||
| def get_host_path(value): | |||
| # convert relative path to absolute path | |||
| value = convert_to_abs_path(value) | |||
| value = os.path.dirname(convert_to_abs_path(value)) | |||
There was a problem hiding this comment.
What if it's a folder?
There was a problem hiding this comment.
os.path.dirname does not change the value if folder is directly supplied.
sample output:
>>> import os
>>> os.path.dirname("/home/anandhu/")
'/home/anandhu'
There was a problem hiding this comment.
That's only if / is at the end right? I think we should explicitly check for a file here.
There was a problem hiding this comment.
That is correct, to be safe, commit 5fb35f3 uses pathlib library to process the paths
| @@ -419,7 +419,7 @@ def get_container_path_script(i): | |||
|
|
|||
| def get_container_path(value, username="mlcuser"): | |||
| # convert relative path to absolute path | |||
| value = convert_to_abs_path(value) | |||
| # if we are trying to mount a file, mount the parent folder. | |||
There was a problem hiding this comment.
relative path was already been converted to absolute path here. But I guess that should be done in get_container_value itself as there is a chance that the relative path of a folder in MLC cache could also be supplied by user which would not be handled properly here.
There was a problem hiding this comment.
Updated in commit 5e9f7e4
|
Currently, the R-GAT workflow is failing due to dgl 2.5.0 pip install error which is already raised here. |
No description provided.