-
Notifications
You must be signed in to change notification settings - Fork 135
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
Libs update #668
Libs update #668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved.... Can you modify the installation wiki to suggest to run the script instead of the conda command?
It would be worth to run the estrablish_conda_script.sh in the make file as well. |
Just out of curiosity, how hard is it not to use conda? As in if you have to compile Python from scratch to support something that you can't use a binary on, it is much easier to use PIP since that comes with Python. Does estrablish_conda_script.sh have a work around to disable it? |
@joshua-cogliati-inl I remember that PIP had problems in Windows in the past (but those problems might have been fixed). How do you think PIP is going to solve the problems we are currently facing? |
Hm, glancing at establish_conda_script.sh it looks like if it does not find a conda command, it just uses the other methods that raven has, so PIP should still work (you just get a complaint every time) So my question is answered since there is a workaround. Let me just say that running scripts to change the environment in a makefile is a challenge since you have to do it for every target, or call a second make. It might be simpler to just revive build_framework script and just always have make call it. |
Given Josh's comments, I think running the library establishment script by default in "make", with an option to opt out via an argument, probably makes a lot of sense. It also might make sense sometime in the future to investigate using a pip environment with library "pipenv" as an alternative for systems that really don't want conda. So far I don't think we've had that request, but I could see it coming up. I'll go ahead and modify the install in the docs and wiki. As far as I know, though, the Windows and Falcon test machines do not use the library script, which we should fix if we reasonably can. |
Job Mingw Test on 2d1808a : canceled by @PaulTalbot-INL keeping device clear |
Job Test mac on 87438f1 : invalidated by @PaulTalbot-INL trying after removing secondary user environment |
Job Test mac on 87438f1 : invalidated by @PaulTalbot-INL again |
Job Test mac on 87438f1 : invalidated by @PaulTalbot-INL testing library removal after adding clean |
Okay, there are differences. One-at-a-time has the following that at-once does not:
The following versions are different between them (one-at-a-time | at-once):
I'll start poking through those difference and see if I can nail down when the fault starts occurring. |
Numpy 1.12.1 is the latest version of numpy I tried that did not result in the fault. 1.13.1 and 1.42.2 both had the issue (on falcon only), so I pinned numpy to 1.12.1. |
…ose-dev-gcc as a test to qsub command
Job Test qsubs on 309059b : invalidated by @PaulTalbot-INL cleared old hdf5 file, see h5py/h5py#712 for possibly-connected problem |
qsub failed for the following reason:
@PaulTalbot-INL Do you have any idea on that? |
@@ -32,6 +32,11 @@ endif | |||
|
|||
CURR_DIR := $(CURDIR) | |||
|
|||
# touch hit.cpp to make sure its time stamp is different than hit.pyx | |||
## this is not a clean solution, but hopefully it prevents asking to use cython | |||
CYTHON_AVOIDANCE_ACTION=$(shell touch $(MOOSE_DIR)/framework/contrib/hit/hit.cpp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add fixme or todo
for the comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have an official use for TODO or FIXME; on the contrary, it's been suggested we do neither. The use of either of these right now is to flag developers that may want to edit the lines being commented. I don't think adding TODO or FIXME is a worthwhile change unless we define a real usage for them in our standards.
MAKE_ARGS=" " | ||
LIBS_MODE=0 # 0 is "create", 1 is "load" | ||
#### FUTURE WORK: | ||
#### combine "make" and "establish_conda_env.sh" into one installation command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see ^
# --raven-libs-name) | ||
# shift | ||
# export RAVEN_LIBS_NAME="$1" | ||
# ;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments lines are for the future work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if it turns out we don't need to environment variable, it would be nice to work all through the command line in the future; however, this option didn't work for an undetermined reason previously.
#### END FUTURE WORK | ||
|
||
if [[ $LIBS_MODE == 0 ]]; then | ||
#### FUTURE WORK: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see ^
crow/crow_python_modules.mk
Outdated
(cd $(CROW_DIR) && unset CXX CC && CFLAGS="$$CFLAGS $(COVERAGE_COMPILE_EXTRA)" && LDFLAGS="$$LDFLAGS $(COVERAGE_LINK_EXTRA)" && export CFLAGS LDFLAGS &&if test `uname` = Darwin; then MACOSX_DEPLOYMENT_TARGET=10.9; export MACOSX_DEPLOYMENT_TARGET; fi && . ./scripts/setup_raven_libs && python ./setup.py build_ext build install --install-platlib=./install) | ||
(cd $(CROW_DIR) && unset CXX CC && CFLAGS="$$CFLAGS $(COVERAGE_COMPILE_EXTRA)" && LDFLAGS="$$LDFLAGS $(COVERAGE_LINK_EXTRA)" && export CFLAGS LDFLAGS &&if test `uname` = Darwin; then MACOSX_DEPLOYMENT_TARGET=10.9; export MACOSX_DEPLOYMENT_TARGET; fi && . ../scripts/establish_conda_env.sh --load && python ./setup.py build_ext build install --install-platlib=./install) | ||
# without mac deployment target | ||
# (cd $(CROW_DIR) && unset CXX CC && CFLAGS="$$CFLAGS $(COVERAGE_COMPILE_EXTRA)" && LDFLAGS="$$LDFLAGS $(COVERAGE_LINK_EXTRA)" && export CFLAGS LDFLAGS &&. ../scripts/establish_conda_env.sh --load && python ./setup.py build_ext build install --install-platlib=./install) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need these commented lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be removed, I think. I'll do that.
echo ... \>\> If this is not the desirable path, rerun with argument --conda-defs \[path\] or remove the entry from raven/.ravenrc file. | ||
fi | ||
fi | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not properly indented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indenting is not part of the bash syntax, and we don't have a standard for bash yet.
tests/cluster_tests/run_mpi_test.sh
Outdated
module load raven-devel-gcc | ||
module load raven-devel | ||
module load MVAPICH2/2.0.1-GCC-4.9.2 | ||
source activate raven_libraries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is conda activate available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining the "conda" functions within a script is challenging (see establish_conda_env.sh) and often isn't worth it. For now, using "source activate" is more efficient; hopefully by Conda 4.6 (or later) the conda devs figure out how they want us to do things; right now both are acceptable.
netname = 'DataSetUnitTest.nc' | ||
data.write(netname,style='netcdf',format='NETCDF4') # WARNING this will fail if netCDF4 not installed | ||
checkTrue('Wrote to netcdf',os.path.isfile(netname)) | ||
# NOTE: due to a cool little seg fault error in netCDF4 versions less than 1.3.1, we cannot test it currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add TODO
or FIXME
here
netname = 'HistorySetUnitTest.nc' | ||
data.write(netname,style='netcdf',format='NETCDF4') # WARNING this will fail if netCDF4 not installed | ||
checkTrue('Wrote to netcdf',os.path.isfile(netname)) | ||
# NOTE: due to a cool little seg fault error in netCDF4 versions less than 1.3.1, we cannot test it currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as previous comment
netname = 'PointSetUnitTest.nc' | ||
data.write(netname,style='netcdf',format='NETCDF4') # WARNING this will fail if netCDF4 not installed | ||
checkTrue('Wrote to netcdf',os.path.isfile(netname)) | ||
# NOTE: due to a cool little seg fault error in netCDF4 versions less than 1.3.1, we cannot test it currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
While it appears the linux test is hanging on the GitHub report, it has passed on civet. Note there was a disruption on GitHub today for about an hour that may have caused this disconnect. |
All the tests are green except the GitHub issue mentioned by @PaulTalbot-INL, and the checklist is satisfied. |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
Closes #666 and moves toward addressing #664
What are the significant changes in functionality due to this change request?
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>
to True.