Organize and add examples #3820

spyridon97 · 2023-09-20T20:31:34Z

This PR performs the following tasks:

organizes existing examples
adds (most of) the examples from ADIOS2-Examples
disables by default BUILD_TESTING, and ADIOS2_BUILD_EXAMPLES
add readme.md for each example directory and provides a suggested order of checking the examples

This resolves #3806 and (partially?) #3795

Fixes: #3806

vicentebolea · 2023-09-20T20:45:45Z

Good work here, few things:

You can reformat the code in bulk with sudo ./scripts/developer/run-clang-format.sh
As we have much more examples than before how about we disable them by default with aDIOS_BUILD_EXAMPLES option defaulting to off but we enable it in our CI builds.

anagainaru · 2023-09-20T23:06:40Z

Good work here, few things:

You can reformat the code in bulk with sudo ./scripts/developer/run-clang-format.sh

As we have much more examples than before how about we disable them by default with aDIOS_BUILD_EXAMPLES option defaulting to off but we enable it in our CI builds.

I would honestly do the same with the tests if possible (-DADIOS2_BUILD_TESTING=OFF). Users don't really need them.

spyridon97 · 2023-09-22T18:06:01Z

@vicentebolea @anagainaru This PR is ready for review. I tried to reformat, but it does not work for python files for some reason locally, i don't even get the same output as the CI machine

~/Programming/Cpp/ADIOS2 organize-and-add-examples ?2 > ./scripts/developer/run-clang-format.sh
[sudo] password for spiros.tsalikis: 
---------- Begin ENV ----------
DISTTAG=f38container
FBR=f38
FGC=f38
HOME=/root
HOSTNAME=f33d9af01d56
OLDPWD=/
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/root/adios2
SHLVL=1
TERM=xterm
_=/usr/bin/env
----------  End ENV  ----------
~/Programming/Cpp/ADIOS2 organize-and-add-examples ?2 >

vicentebolea · 2023-09-22T18:35:06Z

@spyridon97

but it does not work for python files for some reason locally, i don't even get the same output as the CI machine

Yep, the script for python is in scripts/ci/scripts/run-flake8.sh and it does not edit the source code only complains. You will have to edit those files, perhaps a black ./examples will do it, otherwise you might have to go one-by-one and and make sure that they pass.

vicentebolea

Great job here, this is looking good!

The review has taken me a while since it is 406 files changes, I could not go onto details in the added parts, but that is something for the CI. I left some notes. Other notes:

We will need to reformat the shell script too to pass the linters, After you fix the python formatting you will see the errors.
We might have to disable testing in the static builds if they fail since we hit the space limit in github actions runners.
Once you get the smallest and most succint cmake partern for testing that whether the example is part of the adios worktree apply it to all the tests.

Once again thanks for taking care of this, this is indeed a very laborious effort, but it is looking great 👍 I will be tomorrow online for afew hours before I leave for vacations I will have another look then

testing/install/EnginePlugin/CMakeLists.txt

testing/install/EncryptionOperator/CMakeLists.txt

examples/useCases/insituGlobalArrays/CMakeLists.txt

examples/simulations/lorenz_ode/CMakeLists.txt

examples/simulations/heatTransfer/write/IO_adios2.cpp

examples/plugins/engine/CMakeLists.txt

examples/plugins/operator/CMakeLists.txt

examples/plugins/CMakeLists.txt

anagainaru

Nice work, this was a lot of reorganizing ! I tried to look over most of the changes (source files and not cmake) but there were a lot of files modified so I am sure I missed things.

I built ADIOS2 with your branch on my laptop and the Tests were built so the default is not OFF. This needs to be fixed or if you want to leave them, the build scripts in the scripts folder need to have -D BUILD_TESTING=OFF (you removed this).

One additional comment:

In some cases our examples use #if ADIOS2_USE_MPI and in other we check in cmake and do not generate the example if MPI is not present. Is there a reason a code uses one or the other?

This is more a discussion with @vicentebolea / @pnorbert / @eisenhauer, but didn't we decide we want to keep the complex examples in ADIOS2-Examples and import them in the docs from there? I really don't think we need all of these examples in the ADIOS2 repo (Kokkos-{backend} for each backend is a bit much, it makes sense in a repo dedicated to ADIOS2 examples but not in the main library). I am assuming this was discussed when I wasn't there but I really don't think we should bulk up the library with more than basic examples.

examples/basics/variablesShapes/variablesShapes.cpp

examples/ReadMe.md

vicentebolea · 2023-09-23T21:41:16Z

This is more a discussion with @vicentebolea / @pnorbert / @eisenhauer, but didn't we decide we want to keep the complex examples in ADIOS2-Examples and import them in the docs from there? I really don't think we need all of these examples in the ADIOS2 repo (Kokkos-{backend} for each backend is a bit much, it makes sense in a repo dedicated to ADIOS2 examples but not in the main library). I am assuming this was discussed when I wasn't there but I really don't think we should bulk up the library with more than basic example

I understood from our last few conversations that the consensus is to move all the adios2-examples examples to our main repo and merge then with our main repo examples. What I originally proposed was to simply move the readthedocs for adios2-examples our main repo but in the meeting the consensus was to also move the examples itself. The reason was that it is confusing as it is having two sets of examples. At the end of the day i do not consider it a big deal eitherway. Two things to watch out:

We might want to trim down the number of examples.
The examples source coding standards should not be held to the same level as the aDIOS2 source code, this is unnecessary effort and it will slow down the maintenance of them.

Connecting to the previous point, we might want the examples to be a git submodule so that:

We do not have duplication of code and the adios2-examples (criteria 1)
We can have less strict coding standards (Criteria 2)
As it is part of the adios2 main repo too, it will be kept uptodate. (Criteria 3)

The drawback is that we would have to bump the adios2-example commit version time to time, we can add an automatized rule that it does it whenever something is commited in master in adios2-examples.

What do you all think about it?

spyridon97 · 2023-09-25T16:23:03Z

I built ADIOS2 with your branch on my laptop and the Tests were built so the default is not OFF

I fixed that now.

spyridon97 · 2023-09-25T16:23:55Z

In some cases our examples use #if ADIOS2_USE_MPI and in other we check in cmake and do not generate the example if MPI is not present. Is there a reason a code uses one or the other?

Yes, some examples can compile with both mpi on and off, and some can compile only with mpi on.

anagainaru · 2023-09-26T15:10:13Z

Yes, some examples can compile with both mpi on and off, and some can compile only with mpi on.

I was mainly asking if there is a reason for choosing one or the other? It's not a huge deal, we can deal with this some other time.

Norbert convinced me to go with this version of examples in the main repo so I will remove my request for changes.

spyridon97 · 2023-09-27T14:57:36Z

@pnorbert I would appreciate checking that all the simulation examples work as expected along with their scripts.

Requests addressed and Vicente is out for a couple more weeks.

vicentebolea · 2023-10-17T15:27:20Z

@spyridon97 my changes are ready

vicentebolea

Last round of changes, everything else looks good. I have confirmed that we build the examples in every job in the CI. We might have to adjust this later to disable examples in some already slow builds (static builds probably). Make the discussed changes, rebase to the last master and we can have this merged.

Fantastic work @spyridon97 🎉

CMakeLists.txt

vicentebolea · 2023-10-17T22:09:02Z

I have made the changes to speedup the process

spyridon97 · 2023-10-17T22:13:31Z

Last round of changes, everything else looks good. I have confirmed that we build the examples in every job in the CI. We might have to adjust this later to disable examples in some already slow builds (static builds probably). Make the discussed changes, rebase to the last master and we can have this merged.

Fantastic work @spyridon97 🎉

What discussed changes do you refer to that i have not already done?

vicentebolea · 2023-10-17T22:23:53Z

@spyridon97 I have also remove the mark_advance(BUILD_TESTING) since it is a very common CMAKE variable that the user might expect it to see it when doing cmake -L or ccmake.

vicentebolea · 2023-10-17T22:24:24Z

If you approve the latest changes please merge after the CI passes.

1) Make all examples individually compilable 2) Make examples' executable names be consistent 3) Move certain examples in the right folders 4) Rename certain examples for clarity 5) Make all examples installable 6) Fix Python formatting using black 7) Add licence documentation wherever it's missing

1) Make all examples individually compilable 2) Make examples' executable names be consistent 3) Rename certain examples for clarity 4) Make all examples installable 5) Fix Python formatting using black 6) Add licence documentation wherever it's missing

The CI still enables both.

spyridon97 requested review from vicentebolea and caitlinross as code owners September 20, 2023 20:31

spyridon97 force-pushed the organize-and-add-examples branch 2 times, most recently from 0ed3994 to c93e0fb Compare September 22, 2023 17:49

spyridon97 self-assigned this Sep 22, 2023

vicentebolea previously requested changes Sep 23, 2023

View reviewed changes

anagainaru requested changes Sep 23, 2023

View reviewed changes

examples/basics/variablesShapes/variablesShapes.cpp Show resolved Hide resolved

examples/ReadMe.md Show resolved Hide resolved

spyridon97 force-pushed the organize-and-add-examples branch 4 times, most recently from c1ba1c5 to 523f5d9 Compare September 25, 2023 19:37

anagainaru previously approved these changes Sep 26, 2023

View reviewed changes

spyridon97 dismissed anagainaru’s stale review via ebc3133 September 26, 2023 16:59

spyridon97 force-pushed the organize-and-add-examples branch 7 times, most recently from 72efbf2 to 84604fb Compare September 27, 2023 14:07

spyridon97 force-pushed the organize-and-add-examples branch from 84604fb to 65390e3 Compare September 27, 2023 20:02

spyridon97 requested a review from vicentebolea October 3, 2023 18:14

vicentebolea force-pushed the organize-and-add-examples branch from 1145b99 to 4980f81 Compare October 17, 2023 16:28

vicentebolea requested changes Oct 17, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

vicentebolea force-pushed the organize-and-add-examples branch from 4e4a3d0 to 7bad0a6 Compare October 17, 2023 22:19

vicentebolea self-requested a review October 17, 2023 22:19

vicentebolea previously approved these changes Oct 17, 2023

View reviewed changes

vicentebolea dismissed their stale review via aa77e3c October 17, 2023 22:37

vicentebolea force-pushed the organize-and-add-examples branch 2 times, most recently from aa77e3c to bd89672 Compare October 17, 2023 22:57

spyridon97 and others added 8 commits October 17, 2023 19:11

Fix formatting issue in DetectOptions.cmake

fdee65d

Remove testing based on ADIOS2-examples

b881acc

Disable BUILD_TESTING and ADIOS2_BUILD_EXAMPLES by default

bf63c65

The CI still enables both.

Add/Improve the ReadMe.md files in examples directory

2425033

ci: enable tau check

326bb54

cmake: include ctest before detectoptions

2324daf

vicentebolea force-pushed the organize-and-add-examples branch from bd89672 to 2324daf Compare October 17, 2023 23:11

vicentebolea approved these changes Oct 17, 2023

View reviewed changes

spyridon97 merged commit a43d013 into ornladios:master Oct 18, 2023
32 checks passed

vicentebolea mentioned this pull request Oct 18, 2023

Example fails to build in Crusher #3855

Closed

vicentebolea mentioned this pull request Nov 7, 2023

BPChangingShapeWithinStep.Multiblock.BP3 and BP4 tests are flaky #3897

Closed

BrewTestBot mentioned this pull request Apr 4, 2024

adios2 2.10.0 Homebrew/homebrew-core#167957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organize and add examples #3820

Organize and add examples #3820

spyridon97 commented Sep 20, 2023 •

edited by vicentebolea

Loading

vicentebolea commented Sep 20, 2023

anagainaru commented Sep 20, 2023

spyridon97 commented Sep 22, 2023

vicentebolea commented Sep 22, 2023

vicentebolea left a comment

anagainaru left a comment •

edited

Loading

vicentebolea commented Sep 23, 2023

spyridon97 commented Sep 25, 2023

spyridon97 commented Sep 25, 2023

anagainaru commented Sep 26, 2023

spyridon97 commented Sep 27, 2023 •

edited

Loading

vicentebolea commented Oct 17, 2023

vicentebolea left a comment

vicentebolea commented Oct 17, 2023

spyridon97 commented Oct 17, 2023

vicentebolea commented Oct 17, 2023

vicentebolea commented Oct 17, 2023

Organize and add examples #3820

Organize and add examples #3820

Conversation

spyridon97 commented Sep 20, 2023 • edited by vicentebolea Loading

vicentebolea commented Sep 20, 2023

anagainaru commented Sep 20, 2023

spyridon97 commented Sep 22, 2023

vicentebolea commented Sep 22, 2023

vicentebolea left a comment

Choose a reason for hiding this comment

anagainaru left a comment • edited Loading

Choose a reason for hiding this comment

vicentebolea commented Sep 23, 2023

spyridon97 commented Sep 25, 2023

spyridon97 commented Sep 25, 2023

anagainaru commented Sep 26, 2023

spyridon97 commented Sep 27, 2023 • edited Loading

vicentebolea commented Oct 17, 2023

vicentebolea left a comment

Choose a reason for hiding this comment

vicentebolea commented Oct 17, 2023

spyridon97 commented Oct 17, 2023

vicentebolea commented Oct 17, 2023

vicentebolea commented Oct 17, 2023

spyridon97 commented Sep 20, 2023 •

edited by vicentebolea

Loading

anagainaru left a comment •

edited

Loading

spyridon97 commented Sep 27, 2023 •

edited

Loading