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

Test that Cylc Rose workflow config retrieval respect compat mode #2779

Merged
merged 3 commits into from
Jun 12, 2024

Conversation

wxtim
Copy link
Contributor

@wxtim wxtim commented May 10, 2024

Tests for the combined effect of cylc/cylc-rose#322 and cylc/cylc-flow#6097 in closing cylc/cylc-rose#319.

@wxtim wxtim self-assigned this May 10, 2024
@wxtim wxtim marked this pull request as draft May 10, 2024 12:43
@wxtim wxtim changed the base branch from master to 2.1.x May 10, 2024 12:45
@wxtim wxtim requested a review from oliver-sanders May 10, 2024 14:14
@wxtim wxtim marked this pull request as ready for review May 10, 2024 14:14
@wxtim wxtim added this to the 2.1.x milestone May 10, 2024
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Confirmed that this test fails (without the cylc-flow & cylc-rose changes) with the following string appearing in the job.err file:

Output hello-make:succeeded can't be both required and optional

But passes in combination with the associated cylc-flow & cylc-rose branches 👍.

Comment on lines 1 to 3
program hello
write(*, '(a)') 'Hello World!'
end program hello
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this Fortran? My first exposure to it..!

Copy link
Contributor Author

@wxtim wxtim May 15, 2024

Choose a reason for hiding this comment

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

Yes. (honestly, I wouldn't know where to start writing it, but it always seems pretty readable).

This is copied from another test.

@wxtim wxtim changed the base branch from 2.1.x to master May 13, 2024 09:30
@wxtim wxtim requested a review from oliver-sanders May 13, 2024 09:31
@wxtim wxtim modified the milestones: 2.1.x, 2.3.0 May 13, 2024
@oliver-sanders
Copy link
Member

Re-tested against PRs merged into master branches, all good.

@oliver-sanders
Copy link
Member

oliver-sanders commented May 13, 2024

Small problem, although this test works as expected locally, the CI tests have passed before the upstream PRs were merged.

Presumably these tests are skipped in CI?

... Yes:

./t/rose-task-run/45-app-fcm-make-compat_mode.t .................... skipped: "[t]job-host" not defined or not available

@wxtim
Copy link
Contributor Author

wxtim commented May 13, 2024

Small problem, although this test works as expected locally, the CI tests have passed before the upstream PRs were merged.

Presumably these tests are skipped in CI?

... Yes:

./t/rose-task-run/45-app-fcm-make-compat_mode.t .................... skipped: "[t]job-host" not defined or not available

Yes - they rely on having a remote platform setup.

@oliver-sanders
Copy link
Member

I don't think that's right, fcm_make must load the workflow config in order to check whether there is a corresponding fcm_make2 task. If fcm_make2 is present, we would probably expect fcm_make to be local (it's pushing local data).

@wxtim
Copy link
Contributor Author

wxtim commented May 13, 2024

I don't think that's right, fcm_make must load the workflow config in order to check whether there is a corresponding fcm_make2 task. If fcm_make2 is present, we would probably expect fcm_make to be local (it's pushing local data).

Yes, I've fixed the test. The problem was caused by my installing a PoC Yaml Config plugin which recent api changes in Cylc have broken.

@oliver-sanders
Copy link
Member

(tests should pass once upstream PRs are merged)

Copy link
Contributor

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

I can't get the tests to pass locally; I don't think I am set up for FCM correctly

Comment on lines 1 to 3
program hello
write(*, '(a)') 'Hello World!'
end program hello
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this Fortran? My first exposure to it..!

@oliver-sanders
Copy link
Member

What error are you getting (check the job.err file)?

Copy link
Contributor

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

At least I'm not getting the failure that the issue is about with cylc/cylc-rose#322 and cylc/cylc-flow#6097 checked out

@oliver-sanders
Copy link
Member

Upstream PRs are in, so I would expect the tests to pass, but they have failed again, @wxtim could you take a look.

@wxtim
Copy link
Contributor Author

wxtim commented May 15, 2024

Upstream PRs are in, so I would expect the tests to pass, but they have failed again, @wxtim could you take a look.

I don't think that's right, fcm_make must load the workflow config in order to check whether there is a corresponding fcm_make2 task. If fcm_make2 is present, we would probably expect fcm_make to be local (it's pushing local data).

FCM make is local, but apparently needs to be convinced that make 2 is not, even if make 2 is never called: As it was the fcm make 1 task was failing either side of the change.

Can demo that this works locally be commenting and uncommenting
force_compat_mode=get_compat_mode(get_workflow_run_dir(workflow_id)) at around line 65 in cylc/rose/platform_utils.py

[runtime]
[[fcm_make]]
script = rose task-run --app-key=fcm_make --new
[[fcm_make2]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Despite the fcm_make2 not being in the graph, its config is what the fcm_make task is using Cylc Rose to carry out the lookup on, so it needs defined.

prune further crud out of copied test
@wxtim
Copy link
Contributor Author

wxtim commented May 16, 2024

I've changed this test: It now checks that the error logged by the fcm_make task refers to the platform (for fcm_make2) being garbage. Without the fix you instead get an error referring to the graph being wrong (which it is, unless you are in compat mode).

I've pruned away some deadwood caused by the test being a modified copy of t/rose-task-run/26-app-fcm-make-new-mode-with-cont which was obsfuscating some issues in review.

The code which was broken was the code where an fcm_make task looks for the platform where the fcm_make2 continuation task will be run. It doesn't matter whether we have that task in the graph, we just need it in the runtime for the rose to conclude it's a 2 part task and attempt to use the broken routine in Cylc Rose to look up the continuation platform.

Copy link
Contributor

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Test now passes for me. And still fails if using prior commit of cylc-rose without the fix

@oliver-sanders
Copy link
Member

Mac OS failures unrelated (latest run on master)

@oliver-sanders oliver-sanders merged commit b7adb6f into metomi:master Jun 12, 2024
10 of 12 checks passed
@wxtim wxtim deleted the fix.cylc-rose_issue_319 branch June 12, 2024 15:59
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