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

Initial Python API changes for cmake adoption #2737

Closed
wants to merge 2 commits into from
Closed

Conversation

Lestropie
Copy link
Member

@Lestropie Lestropie commented Oct 25, 2023

@daljit46 Will need your assistance on this one.

Closes #2730.

Follows #2689, which has now been merged to dev.

Main changes implemented in first commit are:

  1. Move Python algorithms code from lib/ into src/. This makes more sense to me than any other alternative I've come up with (including doing nothing).

  2. Remove some of the logic in bin/mrtrix3.py for locating the API modules.
    For external projects, we previously had the option of having in the root directory of that project a softlink to the build script of the main MRtrix3 repository. Where this was used, bin/mrtrix3.py would read the contents of that softlink and use it to locate the lib/ directory of the main repository. This is however no longer applicable as build does not even exist.
    My suspicion is that when we get to the point of dealing with external projects post-cmake, the solution here is going to be construction of a softlink to bin/mrtrix3.py. I think the contents of this PR should work in that scenario, but it will need to be evaluated properly when a solution for external projects that also deals with C++ is constructed. PYTHONPATH will also remain a viable backup.

  3. Resolve the imp module deprecation; similar to add support for python importlib to locate MRtrix3 python libraries #2735, except that on dev Python 2 support has been removed (Drop python2 support #2215) so that code branch is no longer required.


Things to do:

  • Modify cmake to deal with the new location of the Python algorithms.
    I had a go at this about a week ago and struggled and ended up discarding my attempt. From memory there are baked-in named variables for bin and lib directories, but no corresponding variable for a src directory, so this needs to be constructed appropriately.

  • Find a solution to the issue of the run module in particular wanting to know the comprehensive set of MRtrix3 executables.
    This can no longer be done reliably at runtime as the directory in which the Python executable resides following installation may contain non-MRtrix3 executables. See Python modifications for cmake #2730 point 3.

  • Test viability of creating a softlink to bin/mrtrix3.py.

  • Check again why the 5ttgen algorithms need to be placed in "_5ttgen/"; this traces back to f0bb21d, and I'm sure it was necessary at the time, but I don't recall exactly what was playing up that forced me to add that little hack.

- Move algorithm code from python/lib/mrtrix3/ to python/src/mrtrix3/.
- Modify python/bin/mrtrix3.py to reflect reduction of number of ways in which the Python API could potentially be found following calling mrtrix3.execute() in an executable command. This additionally includes the transition from the imp module to use importlib due to deprecation; this is a subset of #2735.
- For now, the capturing of the list of MRtrix3 executables in mrtrix3.EXE_LIST is disabled, as it is possible for MRtrix3 to be installed into a location that additionally includes other non-MRtrix3 executables. An alternative solution for this will need to be subsequently implemented.
@daljit46
Copy link
Member

The logic for handling Python files is mostly encompassed in python/CMakeLists.txt. To ensure that the new src directory is handled correctly, we need:

  • Copy the directory in the build folder.
  • Add the files in src as a cleanup target so that when you run ninja clean, those files are removed from the build directory.
  • Ensure that the files in src are installed in the installation prefix path (if needed).

Something like this should work:

diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt
index 46187b89c..f555c197b 100644
--- a/python/CMakeLists.txt
+++ b/python/CMakeLists.txt
@@ -44,11 +44,13 @@ add_custom_command(
     TARGET CopyPythonFiles
     COMMAND ${CMAKE_COMMAND} -E copy_directory
         "${CMAKE_CURRENT_SOURCE_DIR}/lib" "${PROJECT_BINARY_DIR}/lib"
+    COMMAND ${CMAKE_COMMAND} -E copy_directory
+        "${CMAKE_CURRENT_SOURCE_DIR}/src" "${PROJECT_BINARY_DIR}/src"
 )
 
 set_target_properties(CopyPythonFiles 
     PROPERTIES ADDITIONAL_CLEAN_FILES
-        "${PYTHON_BUILD_BIN_FILES};${PROJECT_BINARY_DIR}/lib"
+        "${PYTHON_BUILD_BIN_FILES};${PROJECT_BINARY_DIR}/lib;${PROJECT_BINARY_DIR}/src"
 )
 
 install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bin/
@@ -58,7 +60,9 @@ install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/bin/
     PATTERN ".pyc" EXCLUDE
 )
 
-install(DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/lib/
+install(
+    DIRECTORY 
+        ${CMAKE_CURRENT_SOURCE_DIR}/lib/ ${CMAKE_CURRENT_SOURCE_DIR}/src/
     DESTINATION ${CMAKE_INSTALL_LIBDIR}
     USE_SOURCE_PERMISSIONS
     PATTERN "__pycache__" EXCLUDE

@daljit46
Copy link
Member

daljit46 commented Oct 27, 2023

One thing I would like to comment on is that, in my opinion, setting the PYTHONPATH should not be treated as a backup option, but rather should be the principal way through which a module is located. This variable is a standard across the entire Python ecosystem, and if we have the means to make our workflow work by relying on this, we should embrace it fully and assume it to be our default method. In fact, to deal with the build folder structure, we could even configure CMake to write to the Python files and modify the PATH from within a script (by adding the corresponding directory which contains the Python module).

@Lestropie
Copy link
Member Author

The issue we've had with PYTHONPATH in the past is specific to developers rather than end users. Many of us have multiple parallel MRtrix3 installations using different code branches, and some of these can have modifications to Python scripts and/or API. Therefore, if a Python script is executed, it's vital that the API from the corresponding code branch be utilised.

Updating to a cmake universe:

Perhaps if someone has gone through the cmake installation step, then it would be reasonable to have that added to PYTHONPATH. For developers more likely to be running from the build directory, I don't see how PYTHONPATH could be set for the user without leading to version conflicts.

Though I think you're later suggesting something else. If I'm following that correctly, it's not utilisation of PYTHONPATH but rather hard-coding absolute path for module import at build / installation time, then my current intuition is that I'd prefer the #2741 solution. Because that involves doing such hard-coding within a standalone executable and not modifying the source files, there's the prospect of it working with the use of softlinks at build time, which would allow editing of source code and testing without invoking cmake. So that argument might be better made in contrast to #2741. Hopefully I can have a decent shot at updating cmake to realise that particular vision of structure, and then we can better assess pros and cons.

@jdtournier
Copy link
Member

Just to add a couple of comments on top of @Lestropie's:

One thing I would like to comment on is that, in my opinion, setting the PYTHONPATH should not be treated as a backup option, but rather should be the principal way through which a module is located.

We've discussed this quite a few times in the past, and always came to the conclusion that we shouldn't rely on it. We're far from alone in that particular conclusion by the way - see these blogs on the issues others have with PYTHONPATH, which I think are relevant for us too:

Personally, I think it should be avoided like the plague, for very similar reasons to why LD_LIBRARY_PATH is strongly discouraged (see e.g. here or here)...

@Lestropie Lestropie mentioned this pull request Jun 9, 2024
@Lestropie
Copy link
Member Author

Superseded by #2920.

@Lestropie Lestropie closed this Jun 9, 2024
@Lestropie Lestropie deleted the cmake_python_src branch June 22, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants