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

Specify python version in library filenames #1155

Closed
Silmathoron opened this issue Mar 25, 2019 · 14 comments
Closed

Specify python version in library filenames #1155

Silmathoron opened this issue Mar 25, 2019 · 14 comments
Assignees
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Discussion Still searching for the right way to proceed / suggestions welcome

Comments

@Silmathoron
Copy link
Member

Currently, NEST cannot be installed locally or system-wide for several Python versions because the libraries files are python specific but would all have the same name.
Should we not fix this problem by using the OUTPUT_NAME keyword of the add_library function?
That way it seems that we could add the python version as a suffix and just enable NEST install to be local or system-wide and available for multiple Python versions without interference.

@terhorstd terhorstd added T: Discussion Still searching for the right way to proceed / suggestions welcome ZC: Installation DO NOT USE THIS LABEL ZP: Pending DO NOT USE THIS LABEL I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Apr 1, 2019
@heplesser heplesser added the S: Normal Handle this with default priority label Apr 10, 2019
@heplesser
Copy link
Contributor

@Silmathoron This sounds good! Would you be able to create a PR?

@Silmathoron
Copy link
Member Author

@heplesser yes, I can have a look in the coming weeks

@apeyser
Copy link
Contributor

apeyser commented Apr 10, 2019

So -- how dimensional should the matrix be? Or should this be handled more as a module, with nest being installed in subdirectories that are tagged and added to PATH?

This is the same problem as multiple dependency on HPC systems: you could have NEST with no python and openmpi, NEST with python3.4 and mpich, NEST with python3.7 parampi and so forth, ad infinitum -- so solving this with tagging libraries may not be generally optimal.

@Silmathoron
Copy link
Member Author

Silmathoron commented Apr 10, 2019

@apeyser I was only thinking about Python... given how the python install works, I don't see how to make anything better than that...
To clarify: the python files will always be installed at the same place so you cannot really make different versions which would select different .so files coexist easily

@apeyser
Copy link
Contributor

apeyser commented Apr 12, 2019

You can do installs to different local paths, and then source the correct nest_vars.sh files. That can be integrated with various modules schemes to load the correct combinations of python, mpi, nest and so on.

I strongly suggest that as the better approach here.

So, instead of installing to /usr, install to /usr/libexec/nest2.16-python3.4-openmpi, /usr/libexec/nest2.16-python3.7-mpich/, ... and so on.

Then you can write a Tk file for each (see module) for the various combinations that you like -- I know that module is included in fedora (it's used to select mpi), and probably on most distributions.

@Silmathoron
Copy link
Member Author

@apeyser what you are saying is correct but does not require anything on our part: this is something any user (provided the necessary knowledge) can already do.
Furthermore, such a setup would require additional work or knowledge on the user side because someone would have to decide which install is the default one... I don't really see how what you propose can be done easily (in the sense that it would not require additional work on the user side)

@apeyser
Copy link
Contributor

apeyser commented Apr 17, 2019

Yes -- I'm proposing that module files be added to distributions.
If NEST is built by hand, the user should know what they're doing, and we should put this issue as a suggestion in the INSTALL.txt, in case people don't realize that they can build multiple NESTs by hand.

For distributions, it should be part of the distribution package construction process. That is the standard approach for such problems for good reasons. Some parts are reusable.

@heplesser heplesser removed ZC: Installation DO NOT USE THIS LABEL ZP: Pending DO NOT USE THIS LABEL labels Apr 7, 2020
@jougs
Copy link
Contributor

jougs commented Apr 29, 2020

@Silmathoron: can you please comment on the current state of this? If we're not doing anything, I'd rather close this as wontfix than having it around forever. Thanks!

@Silmathoron
Copy link
Member Author

Silmathoron commented Apr 30, 2020

@jougs I think current situation is suboptimal; I'm not convinced by the modular approach since it relies on user behavior.
I was initially waiting for further opinions, then forgot ^^

Unless anyone strongly disagrees, I would propose to change current CMake config and use:

set_target_properties(libname PROPERTIES OUTPUT_NAME "libname_nest_py${PYTHON_VERSION}")

This leads to no additional maintenance for us and would avoid name conflicts in .local/lib or /usr/lib plus potentially make some debugging easier even if users do not provide all necessary information, and make the multi-python install possible.

@clinssen
Copy link
Contributor

I am a little confused about the use case here, as pynestkernel.so is already installed into a path that includes the Python version, e.g. lib/python3.5/site-packages/nest. In any case, if a user wants to use two NEST installs next to one another, they would just use a custom CMAKE_INSTALL_PREFIX, and they can use nest_vars.sh as currently advertised (which sets PYTHONPATH). So is this change really necessary?

@Silmathoron
Copy link
Member Author

Silmathoron commented Apr 30, 2020

@clinssen this is for the other library files, those that live directly in ${CMAKE_INSTALL_PREFIX}/lib, i.e. potentially /usr/lib or ~/.local/lib

The problem here is that pynestkernel.so is python specific while the others are not (well they are but it's not visible), so installing for py3.6 then py3.5 in the same lib folder would lead to an overwrite of the 3.6 libraries, leading to crash and burn when trying import nest from python 3.6.

Though the use case for multiple python installs (without using env) might be limited, this also avoid issues when upgrading python and makes the python dependency explicit and visible (plus reduces the probability of name conflicts).

@clinssen
Copy link
Contributor

@clinssen this is for the other library files, those that live directly in ${CMAKE_INSTALL_PREFIX}/lib, i.e. potentially /usr/lib or ~/.local/lib

Note that after merging #1513, these all live in the subdirectory nest, i.e. /usr/lib/nest or ~/.local/lib/nest, (almost) eliminating the potential for naming conflicts with non-NEST libraries.

I see your point. I think however that it should be the user's responsibility to use prefixes and nest_vars.sh to manage more than one NEST installation. This is also what is suggested in the documentation. Including the Python version string in all of the libnestkernel.so, libtopology.so and so on filenames seems a little inelegant to me.

Just out of curiosity: does the Python version built for affect these libraries (libnestkernel.so, libtopology.so and so on) in any way whatsoever?

@Silmathoron
Copy link
Member Author

OK, then the "nest" part is definitely not necessary.
As for the python version, yes, it did, last time I checked (though that was a long time ago). I have no idea whether we could actually cut this dependency or not, though... (or whether we already did)

I also have no strong opinion about actually implementing this feature, I think I opened this issue following a number of users' questions that had to do with incorrect python versions or multiple installs, but I don't have a use case myself, so... ;P

@jougs
Copy link
Contributor

jougs commented Apr 30, 2020

Thanks for the discussion. In a consulting call with @clinssen, we decided to close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Discussion Still searching for the right way to proceed / suggestions welcome
Projects
Status: Done
Development

No branches or pull requests

6 participants