[BUG] Hard-coded external library references

[dsroberts]

Describe the bug

Hard-coded paths to directories outside of the application tree are present in CMORise.py in the latest version on the accessnri conda channel. This causes the import to fail if those paths are not available.

To Reproduce

>>> import access_med_utils
  File "<stdin>", line 1, in <module>
  File "/jobfs/122946801.gadi-pbs/analysis3-24.07/lib/python3.10/site-packages/access_med_utils/__init__.py", line 1, in <module>
    from .ilamb import *
  File "/jobfs/122946801.gadi-pbs/analysis3-24.07/lib/python3.10/site-packages/access_med_utils/ilamb.py", line 7, in <module>
    from .CMORise import generate_cmip
  File "/jobfs/122946801.gadi-pbs/analysis3-24.07/lib/python3.10/site-packages/access_med_utils/CMORise.py", line 24, in <module>
    from app_functions import *
ModuleNotFoundError: No module named 'app_functions'

[marc-white]

@dsroberts I don't think this is a hard-coded path issue (those have existed forever in the repo, as far as I can tell) - rather, it seems to be a relative import issue. Altering the offending line to

from .app_functions import *

allows a successful import within ipython. I'll open up a branch to play with it further.

I'm not sure why this has become an issue, unless there has been a change/update to the underlying Python environment that's made it more fussy about relative imports.

[dsroberts]

Hi @marc-white. "Forever" is a bit of a stretch. This particular path was added on the 18th of June, and more importantly, the CI was skipped on that day, so the conda package was not updated at that point. The conda package was updated on the 23rd of July through this action.

Our conda analysis environments (which contained the access-med-tools conda package) was updated on the 22nd of July with this action and again on the day I created this issue with this action. So I didn't get the version with CMORise.py until then. These environments are built in isolation. Our build service user does not have access to kj13, and therefore can never import from that path. I am not in kj13 either, so I can only guess that there might be an app_functions.py on that path that was actually being imported, rather than the app_functions.py that is included in the package. In any case, hardcoded absolute paths in conda packages is not best practice, as conda packages are intended to be portable. Although looking closer, it does appear that none of these paths are used (except accidentally), which I guess raises the question 'why are they there at all?'.

Also, sys.path.append('./') is not a good idea and can lead to undefined behaviour. There is no telling a) what the user's working directory is, and b) what might be in any app_functions.py file in said user's working directory.

[marc-white]

Hi @marc-white. "Forever" is a bit of a stretch. This particular path was added on the 18th of June, and more importantly, the CI was skipped on that day, so the conda package was not updated at that point. The conda package was updated on the 23rd of July through this action.

Ah, that's useful history, I hadn't twigged that CMORise.py was that new of an addition.

Our conda analysis environments (which contained the access-med-tools conda package) was updated on the 22nd of July with this action and again on the day I created this issue with this action. So I didn't get the version with CMORise.py until then. These environments are built in isolation. Our build service user does not have access to kj13, and therefore can never import from that path. I am not in kj13 either, so I can only guess that there might be an app_functions.py on that path that was actually being imported, rather than the app_functions.py that is included in the package. In any case, hardcoded absolute paths in conda packages is not best practice, as conda packages are intended to be portable. Although looking closer, it does appear that none of these paths are used (except accidentally), which I guess raises the question 'why are they there at all?'.

Agree with all. The possibility of a 'floating' definition of app_functions.py is very concerning. @rbeucher is there an app_functions.py in kj13 that would normally be absorbed?

Also, sys.path.append('./') is not a good idea and can lead to undefined behaviour. There is no telling a) what the user's working directory is, and b) what might be in any app_functions.py file in said user's working directory.

Strongly agree, I'll need to work out why that is there.

[rbeucher]

I think that's a question for @rhaegar325 who has been looking into this. I also agree with the comments above. The package should not have access to kj13. It is a project internal to the NRI

[rhaegar325]

Hi, @dsroberts @marc-white Really appreciate to point this error out, this issue is because I did clean up all the development redundancy. now it has already been solved in new branch update-CMORiser.py, waiting to been merged.

[marc-white]

@rhaegar325 has the fix been merged so we can close off this issue?

[marc-white]

@dsroberts are you still seeing this issue?

[dsroberts]

Hi @marc-white. Whilst the package imports successfully now, there is still the issue of a hard-coded path here: https://github.com/ACCESS-NRI/MED-utils/blob/main/access_med_utils/CMORise.py#L17. I've been in contact with the CI of p66 about this, and have been informed that a) MED-utils should not be using these files, b) The purpose of MED-utils as a CMORiser is unclear given NRI's support for ACCESS-MOPPeR and c) a) and b) notwithstanding, these files should be moved to an NRI-controlled location if they're needed on an ongoing basis - which wouldn't solve the issue anyway as they cannot be moved to hh5 where the conda environment in which MED-utils is installed resides. This all represents a larger discussion well beyond the scope of this issue, so I'll close it.

[rbeucher]

Thanks for pointing that out @dsroberts. @rhaegar325 We do not want any hard coded path. Can you look into this please?