Add parsl test

[hategan]

This PR adds a sanity test for Parsl.

[SteVwonder]

Fixes #30.

I don't see any discussion over in #30 on the planned tests. Is this the full set of tests that you plan to include for parsl? Or are there others that you are thinking of adding (e.g., the parsl regression/unit/integration tests).

[hategan]

Fixes #30.

I don't see any discussion over in #30 on the planned tests. Is this the full set of tests that you plan to include for parsl? Or are there others that you are thinking of adding (e.g., the parsl regression/unit/integration tests).

Sorry. I thought #30 was about adding a basic test. Now that I read it again, I'm not quite sure what #30 is about, but likely not this.

[dongahn]

Is this ready to be merged? If so, can a reviewer add MWP?

[dongahn]

Ping

[dongahn]

@SteVwonder:

One of the test instance is complaining about "merge commit"

Run flux-framework/pr-validator@master
2
/usr/bin/docker run --name a33c172ca300caa0e42f4994248b4d9f4e3f4_d135c5 --label 8a33c1 --workdir /github/workspace --rm -e HOME -e GITHUB_JOB -e GITHUB_REF -e GITHUB_SHA -e GITHUB_REPOSITORY -e GITHUB_REPOSITORY_OWNER -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RETENTION_DAYS -e GITHUB_ACTOR -e GITHUB_WORKFLOW -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GITHUB_EVENT_NAME -e GITHUB_SERVER_URL -e GITHUB_API_URL -e GITHUB_GRAPHQL_URL -e GITHUB_WORKSPACE -e GITHUB_ACTION -e GITHUB_EVENT_PATH -e GITHUB_ACTION_REPOSITORY -e GITHUB_ACTION_REF -e GITHUB_PATH -e GITHUB_ENV -e RUNNER_OS -e RUNNER_TOOL_CACHE -e RUNNER_TEMP -e RUNNER_WORKSPACE -e ACTIONS_RUNTIME_URL -e ACTIONS_RUNTIME_TOKEN -e ACTIONS_CACHE_URL -e GITHUB_ACTIONS=true -e CI=true -v "/var/run/docker.sock":"/var/run/docker.sock" -v "/home/runner/work/_temp/_github_home":"/github/home" -v "/home/runner/work/_temp/_github_workflow":"/github/workflow" -v "/home/runner/work/_temp/_runner_file_commands":"/github/file_commands" -v "/home/runner/work/SDK/SDK":"/github/workspace" 8a33c1:72ca300caa0e42f4994248b4d9f4e3f4
3
Validating commits on current branch:
4
 ✘ ebc8912 Merge branch 'master' into add_parsl_test

Do you have any idea why?

[dongahn]

@hategan:

Traceback (most recent call last):
52
  File "/test.py", line 3, in <module>
53
    import parsl
54
ModuleNotFoundError: No module named 'parsl'
55
0a1,3
56
> Hello World from Python!
57
> Hello World!
58
> 
59
Output does not match
60
Error: Process completed with exit code 1.

He doesn't understand why this fails because Parsl should be available in the docker image. He will take a look.

[SteVwonder]

Do you have any idea why?

We added to mergify config's a merge fallback action if the rebase fails. We also use flux's commit verifier action which disallows a merge commit, so once mergify tries the merge, the commit check will fail, causing a PR "deadlock". We should either back out the merge fallback (my vote since it is quickest and if we want to allow merges, the github button is still there) or we can create our own validate commits action that allows merge commits.

[hategan]

The test fails because the test script starts with "#!/usr/bin/python3". However, at some point, the base image started using a VE in /ve_exaworks and this is where all pip packages, including parsl, are installed. These packages are not seen by the system python.

[hategan]

Do you have any idea why?

We added to mergify config's a merge fallback action if the rebase fails. We also use flux's commit verifier action which disallows a merge commit, so once mergify tries the merge, the commit check will fail, causing a PR "deadlock". We should either back out the merge fallback (my vote since it is quickest and if we want to allow merges, the github button is still there) or we can create our own validate commits action that allows merge commits.

We speculated during the meeting that a rebase may solve the issue. That is quite obviously wrong, since merge commits are commits in the branch which get replayed by a rebase. So this is currently a big mess and the easiest solution seems to be scrapping this PR and redoing it with a new branch.

I'm not really familiar with automatic rebasing. In my understanding of https://www.atlassian.com/git/tutorials/merging-vs-rebasing, it seems like a bad idea.

[SteVwonder]

So this is currently a big mess and the easiest solution seems to be scrapping this PR and redoing it with a new branch.

No need to scrap the PR if you don't want to. You can rebase to drop the merge commit that Mergify added and then rebase on top of master.

I'm not really familiar with automatic rebasing. In my understanding of https://www.atlassian.com/git/tutorials/merging-vs-rebasing, it seems like a bad idea.

Yeah, it is a bad idea for public branches that other people are co-developing on/off of because it re-writes history. With C4, you aren't supposed to have branches in the main fork for that reason. And long-lived branches/PRs are kind of an overall git anti-pattern. But yeah, the C4-style gitflows aren't very amenable to people building on top of branches.

EDIT: and in hindsight, the merge fallback action was a bad idea on my part.

[hategan]

With C4, you aren't supposed to have branches in the main fork for that reason.

Isn't this irrelevant if some tool automatically rebases whatever branch the PR is based on?

[SteVwonder]

Isn't this irrelevant if some tool automatically rebases whatever branch the PR is based on?

It only is supposed to do this as the final step before merging. Most of the time, the bot rebases, the CI runs post-rebase, the CI turns green, and the PR gets merged. Only in exceptional circumstances does the PR CI fail post-rebase and require someone to come by and fix things up in the now re-written history. Running git fetch remote-name && git --reset remote-name/branch will make your local working tree look like the new branch contents force-pushed by the bot (which is what your branch would have had to look like anyways if we were strictly enforcing rebasing before a PR merge).

[SteVwonder]

BTW, if we get https://github.com/ExaWorks/SDK/pull/57 merged, this PR can go in as is.

[SteVwonder]

BTW, if we get #57 merged, this PR can go in as is.

Correction. Looks like the CI is using the github workflow yaml in this PR as opposed to the master branch (which makes sense in retrospect). So I think this either needs a merge with or rebase onto master so that the new commit validation can take effect and the CI go green.

[hategan]

BTW, if we get #57 merged, this PR can go in as is.

Correction. Looks like the CI is using the github workflow yaml in this PR as opposed to the master branch (which makes sense in retrospect). So I think this either needs a merge with or rebase onto master so that the new commit validation can take effect and the CI go green.

I rebased locally, but there were quite a few conflicts that had to be resolved manually. Anyway, there's now diverging histories and I would have to force-push to get the rebase in. Are you SURE you want me to do that?

[SteVwonder]

Anyway, there's now diverging histories and I would have to force-push to get the rebase in. Are you SURE you want me to do that?

Yeah, as long as you mean you plan to force push to add_parsl_test, then yes, that is the intended workflow. Rebase your branch onto master, then force-push your branch, then merge the PR (assuming CI passes).

[hategan]

Anyway, there's now diverging histories and I would have to force-push to get the rebase in. Are you SURE you want me to do that?

Yeah, as long as you mean you plan to force push to add_parsl_test, then yes, that is the intended workflow. Rebase your branch onto master, then force-push your branch, then merge the PR (assuming CI passes).

Done. Feel free to merge.

[dongahn]

Ugg... The CI failure is this. It looks to me like they update their release version such that https://apache.claz.org//ant/binaries/apache-ant-1.9.15-bin.tar.gz is no longer valid.

[dongahn]

@j-woz:

apache-ans is Swift/T's dependency (https://github.com/ExaWorks/SDK/blob/master/docker/base/Dockerfile#L80). Is there anyway we can point this to a more stable location?

If nothing else, can you do a quick fixup PR for this?

[SteVwonder]

I ran into the same issue in #51. I have a quick fix in https://github.com/ExaWorks/SDK/pull/51/commits/d50d2821c6c4578530589acfd14c3b9a279906fc. Happy to spin it out into a separate PR if you want.

[dongahn]

@SteVwonder: great. Yes, please put that as a separate PR. That should allow this long-standing PR to be merged.

[dongahn]

@hategan:

PR #59 has gone in. So all you need would be to git fetch upstream, git rebase upstream/master and then force a push.

There is a chance PR #59 could cause a conflict during the rebase but that should be straightforward to resolve. I added merged-when-pass label. So once it is pushed, this will be merged automatically if nothing else goes wrong.

[dongahn]

Oh well. This is kind of silly, but our validator fails this because of a long commit header message:

Can the message header be shorten to:

invoke test.py with explicit python from env

And then in the commit message body, add

Avoid the wrong python being used?

[dongahn]

Awesome!