Test Native and GSL Implementations #1
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mkasberg
force-pushed
the
native-gsl-tests
branch
6 times, most recently
from
62a2db5
to
c68f825
Compare
classifier-reborn is designed to work with or without [GSL](https://www.gnu.org/software/gsl/) support. https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68 If GSL is installed, classifier-reborn will detect it and use it. If GSL is not installed, classifier-reborn will fall back to a pure-ruby implementation. The mechanism for doing so is in `lsi.rb`: https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17 I think it's important to test the classifier-reborn gem with GSL support in CI. One of my goals is to add similar support using Numo, and I'd like that to be tested in CI as well. Also, I want to make sure I don't do anything along the way that could break existing GSL support. As far as I can tell, GSL support was never tested in CI before now (though it was previously discussed [here](jekyll#46 (comment))). In this PR, I'm expanding our text matrix to test with and without GSL enabled. When the matrix has GSL enabled, we install the `gsl` gem (which is not included in our Gemfile since it's optional). Lucky for us, GitHub already [includes libgsl as pre-installed software](https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#installed-apt-packages), so we don't need to do anything special for the apt package. Our gem already has everything needed to build & install. When `matrix.gsl` is false, we won't install the gem and tests will run the native Ruby implementation. When `matrix.gsl` is true, we'll install the gem tests will run the GSL implementation. Currently, GSL only works with Ruby 2.7. (One of the main reasons I want to add support for Numo is because GSL is becoming difficult to support.) As such, I've excluded other versions of ruby in our test matrix for now. They'll still be tested with GSL disabled, but not with it enabled. While working on this, I noticed some tests in the LSI spec that return early when `$GSL` is not enabled. It would be better for those tests to report as skipped when GSL is not enabled (and this matches the pattern of the redis tests, that report as skipped if redis isn't available).
mkasberg
force-pushed
the
native-gsl-tests
branch
from
c68f825
to
ccec8d8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
classifier-reborn is designed to work with or without
GSL support.
https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/docs/index.md?plain=1#L68
If GSL is installed, classifier-reborn will detect it and use it. If GSL
is not installed, classifier-reborn will fall back to a pure-ruby
implementation. The mechanism for doing so is in
lsi.rb:https://github.com/jekyll/classifier-reborn/blob/99d13af5adf040ba40a6fe77dbe0b28756562fcc/lib/classifier-reborn/lsi.rb#L7-L17
Notably, there's a comment there about how to test with/without GSL
enabled.
As far as I can tell, this was only ever used for local
development/testing, and was never tested in CI (though it was
previously discussed
here).
I missed this in my last PR (jekyll#195) because I was focused on porting
existing testing functionality from TravisCI to GitHub Actions. Now that
this is working, I think it's important to expand our CI coverage to
test with and without GSL in CI. So, in this PR, I'm doing so by setting
NATIVE_VECTORto true or false in our test matrix.While working on this, I noticed some tests in the LSI spec that return
early when
$GSLis not enabled. It would be better for those tests toreport as skipped when GSL is not enabled (and this matches the pattern
of the redis tests, that report as skipped if redis isn't available).