-
Notifications
You must be signed in to change notification settings - Fork 198
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
Flux + parsl testing #2713
Comments
Yeah! I’ve implemented flux testing for several tools, and you can use a fairly simple setup to run tests with flux start. Here is one example: https://github.com/reframe-hpc/reframe/blob/develop/.github/workflows/test-flux.yaml |
Hi, @benclifford I'd like to work on this issue. |
@mercybassey ok whats going on in this issue: We have Parsl in this github repository. Parsl executes tasks using components called "executors" - and there are several different executors. Some of them are listed here: https://parsl.readthedocs.io/en/stable/reference.html#executors So @jameshcorbett and @vsoch added an executor that lets Parsl execute tasks through the software that they work on, Flux. You can read more about that here https://github.com/flux-framework and https://computing.llnl.gov/projects/flux-building-framework-resource-management One thing that we've been trying to do more of over the past few years is have more automatic testing of as much as we can, so that for every PR we can have some confidence that we didn't accidentally break something. For some of our executors we have testing: for example, we test install Work Queue on every test run and then test execution with the Work Queue executor - that's driven by a Makefile that does the install and then runs Line 72 in 86a05dd
However, we don't have any testing like this at all for Flux: so I can believe parsl+flux worked it was first contributed by @jameshcorbett but there isn't any further testing that changes in Parsl haven't broken the integration since then. So this issue is about testing the integration of Parsl and Flux to satisfy us that it still works, and in the future keeps working. Some ideas how you could do that: @vsoch pasted a link to a GitHub workflow that starts up Flux from a container and then runs a simple test script. You already saw commands to install Parsl, and the way that we do it in CI is quite similar but written in GitHub Actions so the syntax is quite different - that is defined in this workflow, which is what runs for every pull request: https://github.com/Parsl/parsl/blob/master/.github/workflows/ci.yaml I think a good thing to do would be to take the example yaml that @vsoch pasted above, add it into your Parsl branch in .github/workflows/, make a pull request (perhaps mark it "Draft") and check that that yaml runs (without any Parsl...). Then after that, work on installing Parsl in that yaml. |
I see the goal here is to set up automated testing for the Parsl and Flux integration so it works correctly both now and in the future as changes are made to Parsl. I'll begin reviewing the example workflow, follow your suggestions, and then keep you updated on my progress. |
@benclifford, this was interesting so I gave it a try on #3207 but I had issues: |
@christailu I made a comment on your PR #3207 that it would be good for you to follow along with what is happening on PR #3159 |
Nice! |
@mercybassey thanks for doing the work on this PR - sorry it took a long time to get it all merged. |
Is your feature request related to a problem? Please describe.
The current CI test suite does not test parsl+flux. That means changes to internals of parsl are not tested against flux. This is especially bothering me as I am working on some fairly serious rearrangements of parsl internals and I think it's very likely to break if not tested.
@jameshcorbett and maybe @vsoch do you have any opinions on this?
Describe the solution you'd like
Flux tested against each PR and reporting into github (somewhere: existing parsl CI or somewhere else that reports back to github)
Describe alternatives you've considered
Manual testing. That's extremely fragile.
The text was updated successfully, but these errors were encountered: