search

bandframework / surmise

A python package for surrogate models that interface with calibration and other tools
https://surmise.readthedocs.io/en/latest/
MIT License
11 stars 7 forks source link

Require BAND members to review #88

Closed mosesyhc closed 1 year ago

mosesyhc commented 1 year ago

Instructions for reviewing surmise v0.2.0:

  1. (Local) Follow installation and testing sections under https://surmise.readthedocs.io/en/latest/introduction.html#installation.
  2. (Colab environment) Run the colab notebook.

Let us know if any step needs editing or clarifying. Thanks!

jcyannotty commented 1 year ago

Good News: Google Colab works great!

Bad News: pip install surmise did not work for me on Windows or Ubuntu. Error messages are attached. It appears there is a wheel building issue. This could very well be a "me problem" but I wonder if there is a dependency issue within surmise. Let me know if I can provide any assistance or more information.

surmise_pip_install_error_ubuntu.txt surmise_pip_install_error_windows.txt

mosesyhc commented 1 year ago

Good News: Google Colab works great!

Bad News: pip install surmise did not work for me on Windows or Ubuntu. Error messages are attached. It appears there is a wheel building issue. This could very well be a "me problem" but I wonder if there is a dependency issue within surmise. Let me know if I can provide any assistance or more information.

surmise_pip_install_error_ubuntu.txt surmise_pip_install_error_windows.txt

Thank you John! That's a great catch. Upon looking at the errors, it seems to be the Cython build that requires the Microsoft C build tools.

Meanwhile can you give a compatible Windows .whl a shot? pip install XXX.whl. (https://github.com/bandframework/surmise/releases/tag/v0.2.0)

jcyannotty commented 1 year ago

Good News: Google Colab works great! Bad News: pip install surmise did not work for me on Windows or Ubuntu. Error messages are attached. It appears there is a wheel building issue. This could very well be a "me problem" but I wonder if there is a dependency issue within surmise. Let me know if I can provide any assistance or more information. surmise_pip_install_error_ubuntu.txt surmise_pip_install_error_windows.txt

Thank you John! That's a great catch. Upon looking at the errors, it seems to be the Cython build that requires the Microsoft C build tools.

Meanwhile can you give a compatible Windows .whl a shot? pip install XXX.whl. (https://github.com/bandframework/surmise/releases/tag/v0.2.0)

Okay I have manually downloaded the .whl file and it has successfully installed. I will proceed with testing. You may want to add a line in the documentation that covers this under the installation section.

jcyannotty commented 1 year ago

Update: The ./run-tests.sh does not appear to be working correctly.

Attempt 1: after a successful "pip install surmise" I get this error `jcyannotty@LAPTOP-L6H5TSSB:/mnt/c/Users/johny/appdata/roaming/python/python311/site-packages/tests$ bash ./run-tests.sh

./run-tests.sh: line 1: $'\r': command not found

./run-tests.sh: line 3: $'\r': command not found

./run-tests.sh: line 8: $'\r': command not found

./run-tests.sh: line 9: $'\r': command not found

./run-tests.sh: line 14: $'\r': command not found

./run-tests.sh: line 15: syntax error near unexpected token `$'{\r''

'/run-tests.sh: line 15: usage() {

Attempt 2: Now cloning surmise -- see attached (tried using a virtual env as well) surmise_pytest_error

Any ideas as to what is going wrong?

jcyannotty commented 1 year ago

Note, in the above screen shot, you'll see the pip install pytest command below the error just to verify it was already installed

jcyannotty commented 1 year ago

Update: The ./run-tests.sh does not appear to be working correctly.

Attempt 1: after a successful "pip install surmise" I get this error `jcyannotty@LAPTOP-L6H5TSSB:/mnt/c/Users/johny/appdata/roaming/python/python311/site-packages/tests$ bash ./run-tests.sh

./run-tests.sh: line 1: $'\r': command not found

./run-tests.sh: line 3: $'\r': command not found

./run-tests.sh: line 8: $'\r': command not found

./run-tests.sh: line 9: $'\r': command not found

./run-tests.sh: line 14: $'\r': command not found

./run-tests.sh: line 15: syntax error near unexpected token `$'{\r''

'/run-tests.sh: line 15: usage() {

Attempt 2: Now cloning surmise -- see attached (tried using a virtual env as well) surmise_pytest_error

Any ideas as to what is going wrong?

An updated solution to this pytest issue....

In the run-tests.sh file, you can replace pytest $COV_LINE_SERIAL -k 'not new' with python -m pytest $COV_LINE_SERIAL -k 'not new'. Note, this replacement should be done in 5 spots (lines 125, 135, 145, 158, and 168). Once changing this, I was able to successfully complete the tests.

Here is a reference for the issue I was having: https://stackoverflow.com/questions/35998992/py-test-command-not-found-but-library-is-installed

I think this is a bash issue, i.e. my version of bash did not like the pytest command and needed python -m pytest. Not sure how you want to approach this going forward. A naive suggestion I can offer is to have two separate run-tests.sh files -- one that uses pytest (i.e. the current version) and then a new version which makes the python -m pytest replacement. You can add a line in the readme and docs describing the differences.

There might be better ways to handle this - but unfortunately this seems more of a bash issue that is local to the user.

mosesyhc commented 1 year ago

Thank you John - #90 solves the pytest not found issue by replacing pytest with python -m pytest.

mosesyhc commented 1 year ago

Since a pull request is created for the v03/surmise branch in bandframework, I am closing this issue to avoid duplicate effort. I am reassigning reviewers in that pull request, which will contain all updates from this issue.

@kylegodbey, @jcyannotty, @furnstahl: Would it be okay if I still assign you for that review? If not, feel free to unassign yourself.