`is_mounted_filepath` incorrect logic does not allow different paths inside and outside of container

[esnvidia]

referring to this function in the code:

Skills/nemo_skills/pipeline/utils/mounts.py

Line 43 in d77caab

if path.startswith(mount_dest):

Ex.

mounts:
  - /lustre/path/to/models/cache/huggingface:/cache/huggingface

env_vars:
  - HF_HOME=/lustre/path/to/models/cache/huggingface

>>> import yaml
>>> from nemo_skills.pipeline.utils.mounts import is_mounted_filepath
>>> from nemo_skills.pipeline.utils.cluster import get_env_variables
>>> with open('cluster.yaml', 'r') as f:
...     config = yaml.safe_load(f)
...
>>> env_vars = get_env_variables(config)
>>> env_vars
{'HF_HOME': '/lustre/path/to/models/cache/huggingface', ...}
>>> is_mounted_filepath(config, env_vars['HF_HOME'])
False

Here is the code in the current function:

   mounts = mounts or (get_mounts_from_config(cluster_config) + ["/nemo_run/code:/nemo_run/code"])
    # Find which mount path matches the filepaths prefix
    for mount in mounts:
        mount_source, mount_dest = mount.split(":")
        if path.startswith(mount_dest):
            return True

So I have: /lustre/path/to/models/cache/huggingface:/cache/huggingface

which is split up as:

mount_source= /lustre/path/to/models/cache/huggingface
mount_dest=cache/huggingface\

then checks if HF_HOME=/lustre/path/to/models/cache/huggingface starts with mount_dest=/cache/huggingface

That’s dumb logic - it wants the mount point outside to be the same start string as inside the container.

[Kipok]

I think this works as expected. You should define HF_HOME as a mounted path as it's being used inside container. So if you just change

mounts:
  - /lustre/path/to/models/cache/huggingface:/cache/huggingface

env_vars:
  - HF_HOME=/cache/huggingface

then it should work. Please let us know if it doesn't

[esnvidia]

Thanks, I see that now. One suggestion is to rename the function to better reflect that it's checking the env_var is mounted inside. is_mounted_filepath can refer to "is (bind) mounted to (outside)" container, or "is mounted inside" container. Next, we're checking if the env_var is mounted inside the container. This could be the filepath or a directory. The additional specificity helps user experience. :)

[Kipok]

thanks for the explanation! Do you have a suggestion for how you'd rename that function?

[esnvidia]

is_env_var_mounted_in_container with a docstring and error (or warning) message if an env_var is not found inside the container. I'm a fan of explicit over implicit

[Kipok]

but it's not specific to env vars, right? It's just a generic function that checks if path is mounted in the list of mounts defined in the config. It's used for more than just checking HF_HOME env var. Perhaps we should just improve the error message for when we check for HF_HOME specifically? Do you have suggestions for what we should write there to make it easier to understand and avoid issues like you had?