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

Fix Failing Actions #148

Merged
merged 25 commits into from
Dec 12, 2024
Merged

Conversation

simmsa
Copy link
Contributor

@simmsa simmsa commented Dec 4, 2024

Windows Failing Tests

  • river functions that call delft_3d_get_all_time, on python 3.10, all matlab versions, mhkit-python 0.7.0.
Running River_TestIO_Delft
  ...
  ================================================================================
  Error occurred in River_TestIO_Delft/testGetAllTime and it did not run to completion.
      ---------
      Error ID:
      ---------
      'MATLAB:Python:ConverterError'
      --------------
      Error Details:
      --------------
      Error using py.numpy.ndarray/double
      Conversion of Python 'ndarray' type to MATLAB 'double' is only supported for real numbers and logicals.

      Error in delft_3d_get_all_time (line 26)
          result = double(python_result);

      Error in River_TestIO_Delft/testGetAllTime (line 37)
                  time = delft_3d_get_all_time(data);

Attempted fixes:

  • Update numpy to 1.26.4
    • No change
  • Convert output of d3d.get_all_time to a python list then convert to double

Working fix:

  • Convert np masked array to np array in delft_3d_get_all_time: f7b6e7c

Unix Failing Tests

Error using ssl><module> (line 99)
      Python Error: ImportError:
      dlopen(/Users/runner/miniconda3/envs/mhkit_conda_env/lib/python3.9/lib-dynload/_ssl.cpython-39-darwin.so,
      0x0002): Symbol not found: _X509_STORE_get1_objects
        Referenced from: <DEADD0A1-08C8-3D77-A19C-08827189D194>
        /Users/runner/miniconda3/envs/mhkit_conda_env/lib/python3.9/lib-dynload/_ssl.cpython-39-darwin.so
            Expected in:     <F5E6CE2E-D06E-3F50-8C70-5B9602FE418A>
        /Users/runner/hostedtoolcache/MATLAB/2024.1.999/x64/MATLAB.app/bin/maci64/libcrypto.3.dylib

Starting point:

  • Failing:
    • Macos:
      • 3.9, R2023b
      • 3.9 R2024a
      • 3.10 R2023b
      • 3.10 R2024a
      • 3.11 R2023b
      • 3.11 R2024a

Attempted Fixes:

  • Modify dynamic library paths: b7ce780

Working Fix:

  • Install specific openssl version, 3.0.* with conda and add it as the first entry in DYLD_LIBRARY_PATH: 2ae226e

simmsa added 12 commits December 4, 2024 07:55
River tests that call delft_3d_get_all_time are failing on windows, python 3.10, all matlab versions, mhkit-py
0.7.0.

The tests are failing because this function is being called:
https://github.com/MHKiT-Software/MHKiT-Python/blob/d2a2b66abd663b6184ad5a8d1ce3aac614fb45e2/mhkit/river/io/d3d.py#L9
and Matlab cannot handle output type:

```
Running River_TestIO_Delft
  ...
  ================================================================================
  Error occurred in River_TestIO_Delft/testGetAllTime and it did not run to completion.
      ---------
      Error ID:
      ---------
      'MATLAB:Python:ConverterError'
      --------------
      Error Details:
      --------------
      Error using py.numpy.ndarray/double
      Conversion of Python 'ndarray' type to MATLAB 'double' is only supported for real numbers and logicals.

      Error in delft_3d_get_all_time (line 26)
          result = double(python_result);

      Error in River_TestIO_Delft/testGetAllTime (line 37)
                  time = delft_3d_get_all_time(data);
```

Updating numpy could fix this issue.
This puts the conda environment python libraries before the system
libraries in the dynamic library path. This is meant to fix the
following types of errors where the system is calling a matlab dylib:

```
 Error occurred in Loads_TestExtreme/test_mler_coefficients and it did not run to completion.
      ---------
      Error ID:
      ---------
      'MATLAB:Python:PyException'
      --------------
      Error Details:
      --------------
      Error using ssl><module> (line 99)
      Python Error: ImportError:
      dlopen(/Users/runner/miniconda3/envs/mhkit_conda_env/lib/python3.10/lib-dynload/_ssl.cpython-310-darwin.so,
      0x0002): Symbol not found: _X509_STORE_get1_objects
        Referenced from: <CD4A04CA-EE37-38E7-9488-A3ED4B65EC79>
        /Users/runner/miniconda3/envs/mhkit_conda_env/lib/python3.10/lib-dynload/_ssl.cpython-310-darwin.so
            Expected in:     <F5E6CE2E-D06E-3F50-8C70-5B9602FE418A>
        /Users/runner/hostedtoolcache/MATLAB/2024.1.999/x64/MATLAB.app/bin/maci64/libcrypto.3.dylib
```

More info about dynamic libraries: https://www.hpc.dtu.dk/?page_id=1180
Troubleshooting Windows, Python 3.10 test failures
Fix for this error on windows, python 3.10

```
  Error occurred in River_TestIO_Delft/testGetAllDataPoints and it did not run to completion.
      ---------
      Error ID:
      ---------
      'MATLAB:Python:ConverterError'
      --------------
      Error Details:
      --------------
      Error using py.numpy.ndarray/double
      Conversion of Python 'ndarray' type to MATLAB 'double' is only supported for real numbers and logicals.

      Error in delft_3d_get_all_time (line 26)
          result = double(python_result);

      Error in River_TestIO_Delft/testGetAllDataPoints (line 107)
                  run_timestamps = delft_3d_get_all_time(data);
```
Based on this error:

```
LibMambaUnsatisfiableError: Encountered problems while solving:
  - package numpy-1.26.4-py310h4bfa8fc_0 requires python >=3.10,<3.11.0a0, but none of the providers can be installed

Could not solve for environment specs
The following packages are incompatible
├─ numpy 1.26.4  is installable with the potential options
│  ├─ numpy 1.26.4 would require
│  │  └─ python >=3.10,<3.11.0a0 , which can be installed;
│  ├─ numpy 1.26.4 would require
│  │  └─ python >=3.11,<3.12.0a0 , which can be installed;
│  ├─ numpy 1.26.4 would require
│  │  └─ python >=3.12,<3.13.0a0 , which can be installed;
│  └─ numpy 1.26.4 would require
│     └─ python >=3.9,<3.10.0a0 , which can be installed;
└─ pin-1 is not installable because it requires
   └─ python 3.8.* , which conflicts with any installable versions previously reported.

Pins seem to be involved in the conflict. Currently pinned specs:
 - python 3.8.* (labeled as 'pin-1')
```
@simmsa simmsa marked this pull request as draft December 4, 2024 19:26
simmsa added 13 commits December 4, 2024 12:51
The MHKiT-Python d3d.get_all_time function uses a masked array. This
type is not natively convertible to a double array in matlab. This
handles the conversion in using the python wrapper to ensure that the
result is converted to a compatible numpy array.
Macos errors seem to be related to openssl
Directly calling `matlab` is not allowed:

```
Run MATLABROOT=$(matlab -batch "disp(matlabroot)" | tail -1)
License checkout failed.
License Manager Error -1
The license file cannot be found.

Troubleshoot this issue by visiting:
https://www.mathworks.com/support/lme/R2022b/1

Diagnostic Information:
Feature: MATLAB
License path: /Users/runner/Library/Application Support/MathWorks/MATLAB/R2022b_licenses:/Users/runner/hostedtoolc
ache/MATLAB/2022.2.999/x64/MATLAB.app/licenses/license.dat:/Users/runner/hostedtoolcache/MATLAB/2022
.2.999/x64/MATLAB.app/licenses
Licensing error: -1,359. System Error: 2
mv: rename /bin/maci64/libcrypto.3.dylib to /bin/maci64/libcrypto.3.dylib.bak: No such file or directory
```
@simmsa
Copy link
Contributor Author

simmsa commented Dec 9, 2024

This is working. Summary of changes:

  • Handle edge case in Windows, Python 3.10, MATLAB where a np masked array cannot be parsed
    • Converts np masked array to np array
  • Update MacOS actions to use a specific version of OpenSSL installed by conda that is expected by MATLAB

@simmsa simmsa marked this pull request as ready for review December 9, 2024 19:24
Copy link
Contributor

@hivanov-nrel hivanov-nrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense to me. My only comments which probably stem from me not knowing the finer details:

  • Fixing the github workflows helps our tests pass, but im curious if actual users would experience these issues in their environments. My hope is that things like having an up to date OpenSSL version should be something they already have and is just a weird edge case in our setup.
  • The masked array fix makes sense, my only question if whether we are losing some type of data when forcing the conversion? I'm not sure why it's being used in the first place.

@simmsa
Copy link
Contributor Author

simmsa commented Dec 12, 2024

@hivanov-nrel, good questions.

  1. High level, there is a relatively high likelihood that testing these OS/MATLAB/Python version combinations via GitHub Actions won't be completely reliable due to the complexity of the software stack being tested. While I've been patching issues as they arise, I acknowledge this might not be the optimal strategy. We're balancing between testing all possible user configurations and maintaining reliable CI. For the macOS Actions changes here, my educated guess is that this is isolated to the GitHub Actions environment and wouldn't affect users' machines.

  2. I'm not sure why the masked array is being used either. It could be an artifact of the output from the Delft3D model. However, since our tests are consistent between MHKiT-Python and MHKiT-MATLAB, we should catch any discrepancies in the calculations.

Given that these changes fix the immediate issues with the tests, I'll go ahead and merge this PR.

@simmsa simmsa merged commit caf8263 into MHKiT-Software:develop Dec 12, 2024
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants