Skip to content

Conversation

@JAuriac
Copy link
Contributor

@JAuriac JAuriac commented Feb 20, 2025

Missing the case where paraconf is missing.

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@JAuriac JAuriac linked an issue Feb 20, 2025 that may be closed by this pull request
@jbigot
Copy link
Member

jbigot commented Feb 24, 2025

Warning you're updating the tutorial. This is unrelated

Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

I think this PR is almost ready for merge. Just a few changes are needed.

@JAuriac
Copy link
Contributor Author

JAuriac commented Feb 25, 2025

Warning you're updating the tutorial. This is unrelated

The tutorial previously used broke the CI.

@JAuriac
Copy link
Contributor Author

JAuriac commented Feb 26, 2025

Warning you're updating the tutorial. This is unrelated

The tutorial previously used broke the CI.

Changing back to tutorial @ 7334107.

Yushan-Wang
Yushan-Wang previously approved these changes Sep 19, 2025
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the example being run here. It should, with pdi and no-pdi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example is now run with "PDI_example_trace_C" as part of the PDI tests, then on its own for no-PDI.

@benoitmartin88
Copy link
Member

In addition to my review: it seems like you are testing that the scripts work directly in CI, could you not test locally by using the same docker images as in CI ? This would avoid small intermediate commits.

@JAuriac
Copy link
Contributor Author

JAuriac commented Oct 1, 2025

In addition to my review: it seems like you are testing that the scripts work directly in CI, could you not test locally by using the same docker images as in CI ? This would avoid small intermediate commits.

Related PR : #595

@JAuriac JAuriac requested a review from benoitmartin88 October 1, 2025 13:29
run-local-CI.sh Outdated
@@ -0,0 +1,99 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

Is this file still necessary, as discussed yesterday? I would keep this local, not on git.

@JAuriac JAuriac requested a review from Yushan-Wang October 6, 2025 11:53
Yushan-Wang
Yushan-Wang previously approved these changes Oct 17, 2025
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

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

I'm not sure all the tests and example are run depending on the run configuration.
For instance, the example should run with PDI and no-pdi, with and without paraconf on the system, and this is not currently the case.


cmake -DDIST_PROFILE=Devel ${CMAKE_FLAGS} "${SRCDIR}"
cd "${TEST_DIR_WITH_PDI}"
cmake -DBUILD_BENCHMARKING=OFF -DDIST_PROFILE=Devel ${CMAKE_FLAGS} "${SRCDIR}"
Copy link
Member

Choose a reason for hiding this comment

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

Why deactivate benchmarking ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is temporary, waiting for a definitive commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now reactivated with the following commit.

# Configure, build & test for pdi's example with CMAKE_PREFIX_PATH (find_package) for no-pdi, with Paraconf

cd "${TEST_DIR_NO_PDI}"
cmake -DBUILD_FORTRAN=OFF -DBUILD_DECL_HDF5_PLUGIN=OFF -DBUILD_DECL_NETCDF_PLUGIN=OFF -DCMAKE_PREFIX_PATH="${SRCDIR}/${RELATIVE_NO_PDI_PATH}" "${SRCDIR}/example"
Copy link
Member

Choose a reason for hiding this comment

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

Why deactivate fortran, hdf5 and netcdf plugins ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example's CMakeLists does not use or account for those options, the cmake command here returns a warning of ignored option.


cd "${TEST_DIR_API}"
NO_PDI_TESTS="test_api_C test_api_CXX"
cmake -DBUILD_DECL_HDF5_PLUGIN=OFF -DBUILD_DECL_NETCDF_PLUGIN=OFF -DCMAKE_PREFIX_PATH="${SRCDIR}/${RELATIVE_NO_PDI_PATH}" "${SRCDIR}/tests"
Copy link
Member

Choose a reason for hiding this comment

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

Why deactivate dhf5 and netcdf plugins ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests' CMakeLists does not use or account for those options, the cmake command here returns a warning of ignored option.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is correct. It looks like the example is only run if paraconf is found on the system.
As a reminder, here is what should be run:

n/a PDI no-pdi
paraconf system test PDI, example test no-pdi, exemple
no paraconf system (use embedded) test PDI, exemple test no-pdi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example is already included in the "test PDI" part (so the left column "PDI" is always run).
The if condition of this file only runs the top-right case, to run the example with no-pdi only with the system's paraconf.

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.

Provide a mechanism to deactivate PDI on demand

5 participants