Skip to content

CI: Migrate to GitHub actions #972

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

Merged
merged 13 commits into from
Dec 1, 2020
Merged

CI: Migrate to GitHub actions #972

merged 13 commits into from
Dec 1, 2020

Conversation

effigies
Copy link
Member

@effigies effigies commented Nov 24, 2020

This shifts the majority of CI from Travis to GitHub Actions, and eliminates Azure Pipelines. Azure does not run on maintenance branches, and Travis is now moving to limit builds. Travis is reduced to ARM64 builds.

Part of this effort refactored the Travis scripts to a set of scripts that are hopefully a bit more abstracted from the details of any particular CI, as long as the CI supports bash. Each script should be sourceable or runnable as a standalone script.

The environment variables were the most painful thing to port over. There's probably a better way to do this, and I'm happy to hear any suggestions from those more familiar with Actions.

Closes #971.

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #972 (475ae88) into maint/3.2.x (986af0b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           maint/3.2.x     #972      +/-   ##
===============================================
+ Coverage        91.81%   91.85%   +0.03%     
===============================================
  Files              101      101              
  Lines            12550    12550              
  Branches          2209     2209              
===============================================
+ Hits             11523    11528       +5     
+ Misses             686      683       -3     
+ Partials           341      339       -2     
Impacted Files Coverage Δ
nibabel/volumeutils.py 84.13% <ø> (-0.39%) ⬇️
nibabel/cifti2/parse_cifti2.py 83.86% <100.00%> (ø)
nibabel/cmdline/parrec2nii.py 90.41% <100.00%> (ø)
nibabel/freesurfer/io.py 94.22% <100.00%> (ø)
nibabel/info.py 100.00% <100.00%> (ø)
nibabel/minc1.py 90.64% <100.00%> (ø)
nibabel/minc2.py 89.87% <100.00%> (ø)
nibabel/quaternions.py 99.01% <100.00%> (ø)
nibabel/nifti1.py 92.10% <0.00%> (-0.16%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a84b203...475ae88. Read the comment docs.

@effigies
Copy link
Member Author

effigies commented Nov 24, 2020

Windows failures

__________________________ test_parrec2nii_with_data __________________________

    @script_test
    @needs_nibabel_data('nitest-balls1')
    def test_parrec2nii_with_data():
        # Use nibabel-data to test conversion
        # Premultiplier to relate our affines to Philips conversion
        LAS2LPS = inv_ornt_aff([[0, 1], [1, -1], [2, 1]], (80, 80, 10))
        with InTemporaryDirectory():
            for par in glob(pjoin(BALLS, 'PARREC', '*.PAR')):
                par_root, ext = splitext(basename(par))
                # NA.PAR appears to be a localizer, with three slices in each of
                # the three orientations: sagittal; coronal, transverse
                if par_root == 'NA':
                    continue
                # Do conversion
                run_command(['parrec2nii', par])
                conved_img = load(par_root + '.nii')
                # Confirm parrec2nii conversions are LAS
                assert aff2axcodes(conved_img.affine) == tuple('LAS')
                # Shape same whether LPS or LAS
                assert conved_img.shape[:3] == (80, 80, 10)
                # Test against original converted NIfTI
                nifti_fname = pjoin(BALLS, 'NIFTI', par_root + '.nii.gz')
                if exists(nifti_fname):
                    philips_img = load(nifti_fname)
                    # Confirm Philips converted image always LPS
                    assert aff2axcodes(philips_img.affine) == tuple('LPS')
                    # Equivalent to Philips LPS affine
                contents = fobj.read().strip()
            assert_almost_equal(float(contents), exp_dwell)
            # ensure trace is removed by default
            run_command(['parrec2nii', '--overwrite', '--bvs', dti_par])
            assert exists('DTI.bvals')
            assert exists('DTI.bvecs')
            img = load('DTI.nii')
            bvecs_notrace = np.loadtxt('DTI.bvecs').T
            bvals_notrace = np.loadtxt('DTI.bvals')
            data_notrace = img.get_fdata()
            assert data_notrace.shape[-1] == len(bvecs_notrace)
            del img
            # ensure correct volume was removed
            good_mask = np.logical_or((bvecs_trace != 0).any(axis=1),
                                      bvals_trace == 0)
            assert_almost_equal(data_notrace, data[..., good_mask])
            assert_almost_equal(bvals_notrace, np.array(DTI_PAR_BVALS)[good_mask])
            assert_almost_equal(bvecs_notrace, bvecs_LAS[good_mask])
            # test --strict-sort
            run_command(['parrec2nii', '--overwrite', '--keep-trace',
                         '--bvs', '--strict-sort', dti_par])
            # strict-sort: bvals should be in ascending order
            assert_almost_equal(np.loadtxt('DTI.bvals'), np.sort(DTI_PAR_BVALS))
            img = load('DTI.nii')
            data_sorted = img.get_fdata()
            assert_almost_equal(data[..., np.argsort(DTI_PAR_BVALS)], data_sorted)
            del img
    
            # Writes .ordering.csv if requested
            run_command(['parrec2nii', '--overwrite', '--volume-info', dti_par])
            assert exists('DTI.ordering.csv')
            with open('DTI.ordering.csv', 'r') as csvfile:
                csvreader = csv.reader(csvfile, delimiter=',')
                csv_keys = next(csvreader)  # header row
                nlines = 0  # count number of non-header rows
                for line in csvreader:
                    nlines += 1
    
            assert sorted(csv_keys) == ['diffusion b value number', 'gradient orientation number']
>           assert nlines == 8  # 8 volumes present in DTI.PAR
E           assert 17 == 8
E             +17
E             -8

I don't have a Windows machine to test with. Will ping the Riot to see if anybody does.

Pre-release failures

#968 - waiting on tonight's scipy build

@effigies effigies force-pushed the ci/github_migration branch from c8ef497 to bb431a5 Compare November 24, 2020 15:02
@effigies effigies force-pushed the ci/github_migration branch 4 times, most recently from d61a78e to e042456 Compare November 25, 2020 23:01
@effigies
Copy link
Member Author

This is only failing on pre-releases, now, due to scipy's nightlies. With Travis being what it is, I'm not sure we should wait on those getting fixed.

Anybody care to review? (@yarikoptic?)

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @effigies,

Nice Job! Overall it looks good to me, I just have a small suggestion:

Even if I love Github Actions, one important feature is missing: restart only one failed job. If a matrix failed for whatever reason, you have to restart them all... not convenient and take time... (even if nibabel tests are short).

Because of that, I would suggest splitting test.yml into 3 files, like test_experimental.yml, test.yml, and whatever.yml.
Pros: if one matrice failed, you restart only a small bunch of them
cons: code duplication.

Let me know what you think.

@effigies effigies force-pushed the ci/github_migration branch 6 times, most recently from bb36fcd to 21624b3 Compare November 29, 2020 12:32
@effigies effigies force-pushed the ci/github_migration branch from 21624b3 to 475ae88 Compare November 29, 2020 12:35
@effigies
Copy link
Member Author

@skoudoro Thanks for the suggestion. Broken into three groups:

  1. Main matrix (OS x Python) + minimum requirements checks
  2. Pre-release checks
  3. Style and doctesting the docs

Let me know if you have any other suggestions.

@effigies
Copy link
Member Author

effigies commented Dec 1, 2020

Thanks for the thumbs up. I'm going to go ahead and merge to get tests going again. Curious when Travis will reset our counter...

@effigies effigies merged commit a00dcea into maint/3.2.x Dec 1, 2020
@effigies effigies deleted the ci/github_migration branch December 1, 2020 17:01
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