D3DEnergetic / FIDASIM

A Neutral Beam and Fast-ion Diagnostic Modeling Suite
http://d3denergetic.github.io/FIDASIM/
Other
29 stars 19 forks source link

Add python symlink to dependencies #203

Closed alvin-garcia closed 4 years ago

alvin-garcia commented 4 years ago

@lstagner,

Please merge this in. Since I have not seen any issues from the other scripts, let's leave those with the original path for now.

Note: the anaconda3 folder is installed in /fusion/projects/codes/fidasim/

Thanks.

lstagner commented 4 years ago

This cant be merged in as it has DIII-D specific code. If you want to merge it in, it has to work for all installs.

alvin-garcia commented 4 years ago

Exactly why don't you like it? It works fine and accomplishes the task...

lstagner commented 4 years ago

That type of thinking is why scientific software is usually garbage. The reason I don't like is the hardcoded path to python executable. This is supposed to work for all devices and all installs. Custom installs are common on clusters so the backup you had is not sustainable in the long run.

alvin-garcia commented 4 years ago

I understand why you would prefer the original she-bang line. However, 5 users/developers have encountered issues related to packages and outdated python versions.

An alternative is to remove the she-bang line for run_tests and plot_*, which will force the user to indicate the version of python they are using from the command line, e.g. python plot_inputs . test. This gives an experienced user the option to use a different installation of python. I understand that this doesn't coincide with your current convention, but I can write instructions on our documentation website.

If you insist on keeping the she-bang line, then we are likely to get more messages from users that cannot use our code. I can contact the computing group at DIII-D and NSTX to get the appropriate packages installed, but this is an incomplete solution.

Which option would you prefer?

lstagner commented 4 years ago

Removing the she-bangs isn't ideal but its not a bad solution. I would like to try my original idea of having the user specify the python executable when compiling and creating a symlink. If that doesn't work we can remove the she-bangs

lstagner commented 4 years ago

It should be possible to use a symlink relative to the script location. So I think the best thing to do is to create the symlink in the deps directory when FIDASIM is built. We will default to whatever which python returns and provide a makefile option for users that want to specify a specific python version. This way all the responsibility lies with the user and we don't have to worry about supporting every possible python install on every possible machine.

alvin-garcia commented 4 years ago

Luke,

Please review my new commit. The symlink idea works and I have tested it on Iris. Please let me know if you see any issue. Thanks.

alvin-garcia commented 4 years ago

Luke,

How should we handle this situation? I'm not sure if pushing a python symlink online is possible. I would imagine users create their own executables, develop FIDASIM and then try to push online. We obviously don't want subjective python executables in our online version.

Screen Shot 2020-08-12 at 5 18 59 PM

It is easy for me to remember not to push my executable online. However, it would be nice to make it such that we always reject the python executable from being pushed online, though I am not sure this is possible.

lstagner commented 4 years ago

You just need to append the .gitignore with `deps/python'

alvin-garcia commented 4 years ago

Excellent, that is what I needed. Unless you see anything else, this branch is ready to be merged.

lstagner commented 4 years ago

Your plotting scripts are still using the hardcoded paths and you haven't changed the other python scripts in the lib/scripts directory (submit_fidasim, extract_transp_geqdsk,...)

alvin-garcia commented 4 years ago

Done.