Test with Julienne #169

rouson · 2024-12-28T19:03:40Z

This PR replaces PR #133 and accomplishes the following:

Switch from testing with Veggies to testing with Julienne.
Make progress on issue Unit tests for prif_stop and prif_error_stop make fragile non-portable assumptions #137 by moving the termination tests to a separate fpm project in the new test-termination/ subdirectory as suggested in comment r1761996357.
Define a new prif_test_t type that extends Julienne's test_t type and override test_t's report type-bound procedure, replacing parallel Fortran features with PRIF calls.

The primary motivation for the switch to Julienne is complexity reduction: the Julienne software stack contains 3850 lines of code versus 30513 in Veggies as determined by executing following command on the current main branch of both repositories:

wc -l `find src example test build/dependencies -iname '*.f90'

As a consequence, the test suite runs much faster even after accounting for some possible reduction in test coverage because this test suite was originally developed in 3-4 months ago so any tests added to the Veggies-based test suite since then are not represented here. Determining whether and to what extent the test coverage is actually diminished is on the To Do list below.

To Do

Edit the CI scripts to run the termination tests in the new test-termination subdirectory.
Review and edit the test/ and test-termination/ subdirectories to match the test coverage of the previous Veggies-based tests.

Test coverage: * prif_init * prif_allocate, prif_deallocate, prif_allocate_coarray, prif_deallocate_coarray * prif_co_broadcast, prif_co_max, prif_co_min, prif_co_sum, prif_co_reduce * prif_image_index, prif_num_images, prif_this_image_no_coarray * prif_put, prif_get * prif_sync_all * prif_team_type, prif_form_team, prif_change_team, prif_end_team

This commit replaces veggies and iso_varying_string in the fpm manifest with julienne and assert.

git mv main.{f,F}90

This commit fixes a problem that caused seg faults when running subsets of tests. Previously, if the subset didn't include the prif_init test, then prif_init was never called. This commit calls prif_init in test/main.F90 before running the tests. Consequently, the test for normal execution of prif_init now happens via an assertion rather than in a unit test.

As suggested in the following comment, this commit defines a separate project for testing program termination: #133 (comment)

This commit removes code that was temporarily inserted when diagnosing a flang bug and uncomments the correct code.

This commit defines a new prif_test_t type that extends Julienne's test_t type in ordero to override test_t's "report" type-bound procedure in order to replace parallel Fortran featuers with PRIF calls.

This eliminates a do loop in prif_test_t's "report" tybe-bound procedure.

everythingfunctional

I haven't reviewed the changes the tests yet, but I have a few higher level comments so far.

You need to add an fpm.toml file in the test-termination folder. At present the instructions in the test-termination/README.md do not work. The fpm.toml I created is listed below.
It is unclear from the output whether the tests pass or fail. I would recommend putting together some sort of script that checks the output of each individual test rather than have users try to inspect the outputs and determine pass/fail manually. This should make getting the CI to work easier as well.
I would have made the switch to Julienne a separate PR, as the stop tests don't really depend on the test framework and vice-versa.
I think on balance the switch to Julienne might be the right move, but I think it's worth pointing out some of the important differences:
1. You get less detailed output from running the test suite. i.e. it doesn't report how many or what values are being compared. This could make debugging failing tests (or erroneously passing tests) more difficult.
2. The framework is simpler, and it is one you control.
I think you could adapt veggies in a similar way as you did with julienne, so if you're curious to try it, I could point you at the few things you'd want/need to overload.

example fpm.toml file

name = "test-termination"
version = "0.4.1"
license = "BSD"
author = ["Damian Rouson", "Brad Richardson", "Katherine Rasmussen", "Dan Bonachea"]
maintainer = "[email protected]"
copyright = "2021-2024 UC Regents"

[dependencies]
caffeine = {path = ".."}

[build]
link = ["gasnet-smp-seq", "gcc", "m"]

The new text at the bottom of the test-termination/README.md specifies the environment variable definitions used in running the termination tests.

rouson · 2025-01-02T20:53:14Z

@everythingfunctional thanks for the quick review!

You need to add an fpm.toml file in the test-termination folder. At present the instructions in the test-termination/README.md do not work. The fpm.toml I created is listed below.

Good catch. I just pushed my fpm.toml, which I mistakenly didn't add before. This required git add -f because fpm.toml is in the .gitignore due to the top-level manifest being generated by install.sh.

It is unclear from the output whether the tests pass or fail. I would recommend putting together some sort of script that checks the output of each individual test rather than have users try to inspect the outputs and determine pass/fail manually. This should make getting the CI to work easier as well.

That's the plan. I'm hoping @ktras can work on this.

I would have made the switch to Julienne a separate PR, as the stop tests don't really depend on the test framework and vice-versa.

I agree. If we decide against the switch to Julienne, I'll create a separate PR to replace the old program termination tests with the new ones.

I think on balance the switch to Julienne might be the right move, but I think it's worth pointing out some of the important differences:

You get less detailed output from running the test suite. i.e. it doesn't report how many or what values are being compared. This could make debugging failing tests (or erroneously passing tests) more difficult.

This morning's release of Julienne 1.6.0 adds this information in diagnostic output. This issue has been bugging me for a while, but it took some focused time over the break to think of how to add such diagnostic output in backwards-compatible way. Of course, there aren't many projects using Julienne yet (two-ish) but it would be nice to not have to rewrite multiple projects' test suites all at once. Fortunately I finally figured out a backwards-compatible path and that's in Julienne 1.6.0.

The framework is simpler, and it is one you control.

I think you could adapt veggies in a similar way as you did with julienne, so if you're curious to try it, I could point you at the few things you'd want/need to overload.

Very cool. It would probably be useful info for learning purposes either way so pointers are welcome.

bonachea

Made some initial observations based on a quick skim of the high-level changes.

I have not yet dived into review of the test code.

test/prif_co_reduce_test_m.F90

test/main.F90

bonachea · 2025-01-02T20:57:30Z

manifest/fpm.toml.template

-veggies = {git = "https://gitlab.com/everythingfunctional/veggies", tag = "v1.1.3"}
-iso_varying_string = {git = "https://gitlab.com/everythingfunctional/iso_varying_string.git", tag = "v3.0.4"}
+julienne = {git = "https://github.com/berkeleylab/julienne.git", tag = "1.5.3"}
+assert = {git = "https://github.com/berkeleylab/assert.git", tag = "2.0.0"}


If we are adding a project dependence on berkeleylab/assert (which I DO support), then we should also excise the current caffeine_assert_s module which was added as a temporary workaround for a lack of a real assertion library.

However this change is also orthogonal to the Julienne transition and feels like it belongs in a separate PR.

As agreed in our meeting, I'll be working on this upgrade as a separate PR

bonachea · 2025-01-02T20:59:57Z

test-termination/fpm.toml

I'm not yet convinced that the proposed change fully resolves all six of the problems outlined in issue #137 that this PR claims to solve. However it might be a step in the right direction?

@bonachea if we agree that the new approach is a step in the right direction, then it might make sense to merge the termination-tests as is before more thoroughly addressing #137. How does that sound?

I agree we shouldn't hold up this PR waiting for a complete solution to #137. I've changed the PR description to stop claiming a complete fix to that issue, so that GitHub hopefully won't automatically close it.

I'm not yet prepared to approve a merge of this PR as-is, let's plan to discuss on Wednesday.

This commit appends `_c_bool` to the `quiet` argument of `prif_co_reduce` and removes the `pure` attribue from the encompassing procedure.

rouson · 2025-01-06T20:13:28Z

As of commit fcf2fa8, this PR now adds the capability for diagnostic output in the event of a test failure for all tests except those in test/prif_allocate_test_m.F90 . Adding diagnostic output to the latter test will likely require a relatively minor update to Julienne. That means 48 of 52 tests can produce diagnostic output. I think that's a sufficiently large fraction that the PR could be merged before adding the diagnostic output in the four missing cases.

bonachea

Made some additional observations.

Right now my biggest concern with this PR is that it updates an (old?) snapshot of of the test suite, which is continuing to evolve in several other in-flight PRs. I don't want this PR to become a blocker for merging PRs that add Caffeine features (with corresponding test code that naturally will semantically conflict with this PR).

Given a choice between reaching PRIF 0.5 compliance (a client-facing feature goal) and replacing an internal test harness, I'd currently lean towards prioritizing the former. Does the proposed switch from Veggies to Julienne provide any direct benefit to library clients, or is this purely a maintainability upgrade that mostly benefits Caffeine developers?

Let's plan to discuss this further in our meeting this week.

bonachea · 2025-01-06T21:13:04Z

test/prif_test_s.f90

+
+  impure elemental subroutine co_all(boolean)
+    logical(c_bool), intent(inout) :: boolean
+    call prif_co_reduce(boolean, c_funloc(both))


This is using the PRIF 0.4 version of prif_co_reduce, and will break when we upgrade the implementation to PRIF 0.5, which significantly complicates the code needed to invoke prif_co_reduce (see PR #158).

Moreover there's no semantic reason to use a custom reduction function here. Please convert the boolean to an integer and use prif_co_sum or prif_co_max instead, to avoid the mess mentioned above.

bonachea · 2025-01-06T21:19:35Z

test/prif_co_reduce_test_m.F90

Changes in this file massively conflict with the complete rewrite in-progress in PR #158 to accommodate the PRIF 0.5 changes to collectives.

bonachea · 2025-01-06T21:20:04Z

test/prif_co_max_test_m.F90

Changes in this file will conflict with the changes in-progress in PR #158 to accommodate the PRIF 0.5 changes to collectives.

bonachea · 2025-01-06T21:20:14Z

test/prif_co_min_test_m.F90

Changes in this file will conflict with the changes in-progress in PR #158 to accommodate the PRIF 0.5 changes to collectives.

bonachea · 2025-01-06T21:20:39Z

test/prif_co_sum_test_m.F90

Changes in this file will conflict with the changes in-progress in PR #158 to accommodate the PRIF 0.5 changes to collectives.

bonachea · 2025-01-06T21:30:24Z

test/prif_teams_test_m.F90

Changes in this file massively conflict with the changes in-progress in PR #171, where this test was greatly expanded to complete coverage for the PRIF 0.5 implementation of teams.

bonachea · 2025-01-16T23:37:46Z

The simple file renames for test/* proposed as part of this PR have now been merged in standalone PR #174, as previously discussed and agreed.

This PR will eventually need a rebase to resolve that conflict and also semantic conflicts with PR #171 and #175 that have also been merged. However as discussed work on this PR should probably be deferred until after PR #158 is completed and the test suite reaches a point of stability with PRIF 0.5, to avoid another redundant round of conflict resolution.

rouson mentioned this pull request Dec 28, 2024

Unit test with Julienne #133

Closed

16 tasks

rouson force-pushed the test-with-julienne branch from 189c949 to 886233a Compare December 28, 2024 19:46

rouson added 8 commits December 28, 2024 23:40

chore(fpm.toml): switch dev-dependencies

7ebab05

This commit replaces veggies and iso_varying_string in the fpm manifest with julienne and assert.

chore(test): preprocess test main

1d87e74

git mv main.{f,F}90

test(stopis): def new project to test termination

b0354e1

As suggested in the following comment, this commit defines a separate project for testing program termination: #133 (comment)

doc(test-termination): update README.md

b425d2f

doc(test-termination): update README.md

ad8f546

rm(co_reduce_test): rm temporary code

435d148

This commit removes code that was temporarily inserted when diagnosing a flang bug and uncomments the correct code.

rouson force-pushed the test-with-julienne branch from 886233a to 435d148 Compare December 29, 2024 04:51

rouson added 4 commits December 29, 2024 01:07

test: extend test_t, override report binding

ce8aa06

This commit defines a new prif_test_t type that extends Julienne's test_t type in ordero to override test_t's "report" type-bound procedure in order to replace parallel Fortran featuers with PRIF calls.

fix(co_min_test): simplify logic

c2b765f

fix(com_max_test): borrow logic from co_min_test

3f26d78

fix(co_reduce_test): improve logic

dec2d23

rouson marked this pull request as draft December 29, 2024 07:49

rouson marked this pull request as ready for review December 29, 2024 07:53

rouson requested review from everythingfunctional, bonachea and ktras December 29, 2024 08:04

rouson added 2 commits December 29, 2024 22:42

refac(prif_test_s): make co_all impure elemental

7268613

This eliminates a do loop in prif_test_t's "report" tybe-bound procedure.

chore(prif_test_s): rm redundant logic in report

2ffdbb6

everythingfunctional reviewed Jan 2, 2025

View reviewed changes

rouson added 2 commits January 2, 2025 12:38

doc(test-termination/README): specify environment

7ce33b7

The new text at the bottom of the test-termination/README.md specifies the environment variable definitions used in running the termination tests.

build(test-terminatio): add fpm.toml file

3b5c974

bonachea requested changes Jan 2, 2025

View reviewed changes

rouson added 3 commits January 2, 2025 14:30

build: rm external assert lib dependence

dd3fb3a

chore: rm unused parameter declarations

00ae2b3

test(co_reduce): fix standard violations

6d03002

This commit appends `_c_bool` to the `quiet` argument of `prif_co_reduce` and removes the `pure` attribue from the encompassing procedure.

rouson added 8 commits January 2, 2025 22:12

test(this_image): report diagnostics if test fails

9ae13e9

test(num_images): report diagnostics if test fails

1da811b

test(init): report diagnostics if test fails

9ab0a7f

test(image_index): report diagnostics for failures

5cc1538

test(co_broadcast):report diagnostics for failures

9341364

test(co_sum): report diagnostics for test failures

a529490

test(co_min): report diagnostics for test failures

277c746

test(co_max): report diagnostics for test failures

b3e90e5

bonachea mentioned this pull request Jan 4, 2025

Begin illustrating how to splice into veggies to work with prif #170

Closed

rouson added 4 commits January 3, 2025 22:28

test(teams): report diagnostics for test failures

d70134e

test(rma): report diagnostics for test failures

e892ce2

build(fpm): upgrade dependency to julienne 1.7.0

8a74c08

test(co_reduce): report test failure diagnostics

fcf2fa8

bonachea requested changes Jan 6, 2025

View reviewed changes

bonachea mentioned this pull request Jan 16, 2025

Refactored FORTRAN module #173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test with Julienne #169

Test with Julienne #169

rouson commented Dec 28, 2024 •

edited by bonachea

Loading

everythingfunctional left a comment

rouson commented Jan 2, 2025

bonachea left a comment

bonachea Jan 2, 2025

bonachea Jan 6, 2025

bonachea Jan 2, 2025

rouson Jan 6, 2025

bonachea Jan 6, 2025

rouson commented Jan 6, 2025

bonachea left a comment

bonachea Jan 6, 2025

bonachea Jan 6, 2025

bonachea Jan 6, 2025

bonachea Jan 6, 2025

bonachea Jan 6, 2025

bonachea Jan 6, 2025

bonachea commented Jan 16, 2025

Test with Julienne #169

Are you sure you want to change the base?

Test with Julienne #169

Conversation

rouson commented Dec 28, 2024 • edited by bonachea Loading

To Do

everythingfunctional left a comment

Choose a reason for hiding this comment

rouson commented Jan 2, 2025

bonachea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouson commented Jan 6, 2025

bonachea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonachea commented Jan 16, 2025

rouson commented Dec 28, 2024 •

edited by bonachea

Loading