[MATLAB] Add multi-OS MATLAB test workflows to CI#1974
[MATLAB] Add multi-OS MATLAB test workflows to CI#1974ischoegl merged 4 commits intoCantera:mainfrom
Conversation
ischoegl
left a comment
There was a problem hiding this comment.
I'd prefer utilizing runners that build on previously compiled code within a single matrix job, similar to the approach taken by the .NET runners. Of course, this requires the generated CLib.
46bb57e to
dbdf45d
Compare
b2c110a to
5d6cc8b
Compare
|
Finally got the MATLAB matrix job to run with artifacts (🎉) - turned out to be a little more complicated than .NET, as I had to resolve some naming issues on Linux/macOS, and use a Windows DLL that was compiled outside of conda. A couple of caveats:
Regarding slow samples, it appears that the tagging with |
3b904d9 to
665b5b1
Compare
|
@speth ... after rebasing to switch to the generated CLib and updating the CI logic, I believe this is actually ready (I updated this PR, which was originally opened by @ssun30; the initial work pointed in a lot of the right directions). I believe that I found a happy middle ground for MATLAB testing - run examples on PS: I pushed some simplifications |
665b5b1 to
92e4417
Compare
b531d40 to
23c9a4e
Compare
There was a problem hiding this comment.
Had to go back and fix an issue with a runner failing silently. Based on #2030, I further added more MATLAB versions to the tests. R2024a/b and R2025a work, but R2023b failed. Fwiw, the tests with issues reported in #2034 are now disabled.
The inclusion of older MATLAB versions surfaced another issue caused by Linux glibc, see #1935.
3c118d9 to
127b444
Compare
.github/workflows/main.yml
Outdated
| - name: Run tests and examples (macOS) | ||
| uses: matlab-actions/run-tests@a80b208946040c701ae65c1bce73ba7ec4810757 # v2.1.2 | ||
| with: | ||
| select-by-folder: test/matlab_experimental | ||
| if: runner.os == 'macOS' |
There was a problem hiding this comment.
Given the macOS package doesn't suffer from library compatibility issues (at least as it's built in this CI process), can we do this run using the inprocess mode?
There was a problem hiding this comment.
This is an excellent point, but I'd prefer to handle this in a follow-up PR - I had created #2034 for that reason. It should be addressed for any usage, not just the CI.
| - name: Set LD_PRELOAD environment variable for MATLAB (Linux) | ||
| run: | | ||
| LIB_STDCXX=$(ldconfig -p | grep libstdc++.so.6 | awk '{print $4}' | head -n 1) | ||
| LIB_OPENBLAS=$(ldconfig -p | grep libopenblas.so.0 | awk '{print $4}' | head -n 1) | ||
| LIB_LAPACK=$(ldconfig -p | grep liblapack.so.3 | awk '{print $4}' | head -n 1) | ||
| echo "LD_PRELOAD=$LIB_STDCXX:$LIB_OPENBLAS:$LIB_LAPACK" >> $GITHUB_ENV |
There was a problem hiding this comment.
Is this still necessary, with the outofprocess mode available on Linux (even with its performance limitations)? If this is still required, then we haven't solved any of the library compatibility problems.
There was a problem hiding this comment.
I don't know. Again, I'd prefer to handle this in a follow up that focuses on LD_PRELOAD - see new issue #2035.
| - name: Create library symlinks (Linux) | ||
| working-directory: build/lib | ||
| run: | | ||
| ln -s libcantera_shared.so libcantera_shared.so.3.2.0 | ||
| ln -s libcantera_shared.so.3.2.0 libcantera_shared.so.3 |
There was a problem hiding this comment.
Why are these symlinks required? Can't the Matlab interface just load the unversioned library?
There was a problem hiding this comment.
MATLAB is unable to resolve these. It requires libcantera_shared.3.2.0.dylib for macOS and libcantera_shared.so.3 for Linux. This is the reason why there libraries need to be resolved by extra logic. Took me a while to figure out why @ssun30 had introduced this logic - I updated in the hope to make it easier to follow.
|
|
||
| scores = arrayfun(@(f) versionScore(f.name), files); | ||
| hasVersion = scores > 0; | ||
| if any(hasVersion) | ||
| [~, idx] = max(scores); | ||
| str = string(fullfile(libDir, files(idx).name)); | ||
| else |
There was a problem hiding this comment.
I realize this was a preexisitng condition, but I think this mechanism of interacting with the versioned shared libraries is completely ineffective. There is nothing to make sure that the Matlab toolbox corresponds to the version of libcantera that is found here. In the absence of a more robust mechanism for this, I would suggest just taking the simple approach and linking to the unversioned library name.
I also have no idea what a version "score" is, but that's sort of a moot point compared to the larger issue here.
There was a problem hiding this comment.
It's targeting a different purpose - making sure that MATLAB finds a library that it is able to load. I'll add a comment to the version score (in effect, a way to ensure that we can sort version numbers according to integers).
Ensuring that libcantera matches would have to be tacked in ctLoad. Once we can access ctVersion from the shared library, it can be checked.
Co-authored-by: ischoegl <ischoegl@lsu.edu> Combines: - [CI] Added multi-OS MATLAB test workflows - [CI] Consolidated multi-OS MATLAB tests into one - Update to use artifacts for testing
127b444 to
a68ab13
Compare
Thanks for the review! I took care of the straightforward suggestions. Regarding simplifications, it appears that some of the apparently complex functions are required to make things run. I had already opened #2034 and #2035 to track associated issues. I'd prefer to push |
Changes proposed in this pull request
Checklist
scons build&scons test) and unit tests address code coverage