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

Run xfel-regression on Azure #1707

Merged
merged 19 commits into from
May 18, 2021
Merged

Run xfel-regression on Azure #1707

merged 19 commits into from
May 18, 2021

Conversation

Anthchirp
Copy link
Member

Adds the xfel-regression test pipeline laid out in #1671 to pull request builds only, and in such a way as to

  1. minimize total build time, and
  2. separating out regular test failures from xfel-regression test failures

At this time this is work in progress, especially to test out the pull-request build behaviour, but on merging will also close #1671.

@Anthchirp Anthchirp self-assigned this May 16, 2021
For whatever reason this leads to the stage outcome to not be set to
'success' although the job outcome is shown as successful either way.
This is probably an Azure bug, but given my experience with more
complicated issues (ie. the broken cache issue) it's not worth reporting
as it won't get beyond first level support.
@Anthchirp
Copy link
Member Author

Anthchirp commented May 17, 2021

New issue after this is merged:

This dials test is not run during as part of the main tests, as xfel_regression is not available at this point:

SKIPPED [1] test/algorithms/integration/test_kapton.py:20: test requires xfel_regression

It is also not run as part of the PR builds where xfel_regression is available, as it is not part of the xfel_regression test suite.
So we never run this test here.

I would suggest that either the test is moved into xfel_regression, or the test data is moved into dials-data. At ~6.6 MB the latter shouldn't really be an issue, all it really needs is a description file.

for a slightly nicer display on the GitHub summary page
@Anthchirp Anthchirp requested a review from phyy-nx May 17, 2021 08:52
@Anthchirp Anthchirp marked this pull request as ready for review May 17, 2021 08:53
@Anthchirp
Copy link
Member Author

This is now ready for review.

xfel-regression tests are run after the DIALS tests passed, and in PR builds only. The output is aggregated and reported in a separate build job. I also added another PR job stage ("Summary: Ready to merge") that summarizes the cross-platform dials tests without the xfel-regression tests.

I note that the xfel-regression tests currently fail on MacOS, I'm not sure whether they are supposed to, or not. We can disable the xfel-regression test on MacOS for now and address this in a separate issue/PR.

@phyy-nx
Copy link
Member

phyy-nx commented May 17, 2021

Thanks! This is great, appreciate the effort.

The failure on macos is the cosym test which uses MPI (It does pass on linux). The cctbx_project macos Azure XFEL CI pipeline skips the test as it doesn't install mpi4py. I've essentially never done MPI stuff on macos, so I'd say it's safe to skip that test on macos.

The rest of the tests pass on macos so I'll just add a macos check to the run_tests.py in xfel_regression.

I'll work on test_kapton.py separately.

@phyy-nx
Copy link
Member

phyy-nx commented May 17, 2021

Ok, cosym test updated to only run on linux. Ready to re-run the tests, but I'm not sure if I have the access to do so.

@phyy-nx
Copy link
Member

phyy-nx commented May 17, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Anthchirp
Copy link
Member Author

Glad to hear!

(The easiest way to force a rerun of the PR build only is to just close and reopen the pull request.)

@phyy-nx
Copy link
Member

phyy-nx commented May 17, 2021

Gotcha thanks. Looks like a clean build!

@Anthchirp
Copy link
Member Author

kapton stuff -> #1712, will merge this here

@Anthchirp Anthchirp merged commit a61f2a3 into main May 18, 2021
@Anthchirp Anthchirp deleted the xfel_azure branch May 18, 2021 07:47
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.

None yet

3 participants