Skip to content
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

InstalledCode: Allow relative path for filepath_executable #5879

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jan 31, 2023

Fixes #5867

The InstalledCode required the filepath_executable to be an absolute path. The original reasoning behind this design choice was that this improves the provenance of data produced with this code slightly as it would be more difficult to accidentally change the version of the code that was associated with the executable. For relative paths, the actual linked code could be easily changed through, for example, environment variables.

However, this restriction also has downsides. On many compute clusters the preferred approach is to load specific modules that provide the software of interest in which relative executables are provided. The absolute paths can change depending on microarchitecture of the system or other system variables. Requiring the user to provide the absolute path to the executable defeats the purpose of this system.

Therefore the requirement of the executable path having to be absolute is dropped.

@sphuber sphuber requested a review from ltalirz January 31, 2023 13:12
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sphuber !

just one question.

Also, if you haven't done so already, could you please do a grep on the codebase to check whether there are other instances of 'absolute' that should be removed from the docs?

:raises `~aiida.common.exceptions.ValidationError`: if no transport could be opened or if the defined executable
does not exist on the remote computer.
"""
if not self.filepath_executable.is_absolute():
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checking: what effect does this have in verdi computer test (or wherever this validation is used)?

will the check be shown as skipped or as passed?

Copy link
Contributor Author

@sphuber sphuber Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is only run by verdi code test. It will show that all tests have passed. But this is also the case for codes that are not InstalledCode even though no test was run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sphuber I would note that ideally what you want to do, if the path is not absolute, is load the same environment that you would have, then check that the executable is on the $PATH.
Perhaps there could be an option in the code test, to run the prepend text (from both computer and code) then look for the executable
this would be a more "complete" test of whether the code might have issues running

@sphuber
Copy link
Contributor Author

sphuber commented Jan 31, 2023

Also, if you haven't done so already, could you please do a grep on the codebase to check whether there are other instances of 'absolute' that should be removed from the docs?

I did do a search, which is how I found the section that was updated in the PR.

@ltalirz ltalirz self-requested a review January 31, 2023 16:26
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to go ahead.

If you like, it may be useful to actually change one of the paths of the codes in the tests to no longer be absolute to make sure this works

The `InstalledCode` required the `filepath_executable` to be an absolute
path. The original reasoning behind this design choice was that this
improves the provenance of data produced with this code slightly as it
would be more difficult to accidentally change the version of the code
that was associated with the executable. For relative paths, the actual
linked code could be easily changed through, for example, environment
variables.

However, this restriction also has downsides. On many compute clusters
the preferred approach is to load specific modules that provide the
software of interest in which relative executables are provided. The
absolute paths can change depending on microarchitecture of the system
or other system variables. Requiring the user to provide the absolute
path to the executable defeats the purpose of this system.

Therefore the requirement of the executable path having to be absolute
is dropped.
@sphuber sphuber force-pushed the fix/5867/code-installed-executable-absolute branch from 21149a0 to 1ebf91b Compare February 1, 2023 21:55
@sphuber
Copy link
Contributor Author

sphuber commented Feb 2, 2023

Feel free to go ahead.

If you like, it may be useful to actually change one of the paths of the codes in the tests to no longer be absolute to make sure this works

Thanks @ltalirz . I have added a test.

@sphuber sphuber merged commit d98b6d9 into aiidateam:main Feb 2, 2023
@sphuber sphuber deleted the fix/5867/code-installed-executable-absolute branch February 2, 2023 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drop requirement for Code executable paths to be absolute
3 participants