GEOS-DEV / GEOS

GEOS Simulation Framework
GNU Lesser General Public License v2.1
203 stars 80 forks source link

Add Python Style CI Check #1982

Open cssherman opened 1 year ago

cssherman commented 1 year ago

Now that we've enabled yapf python formatting in cmake, we should add the equivalent CI check. To run the check, the cmake should have a line pointing to the yapf executable, which can be installed in python via pip. For quartz builds, we have the following:

set(YAPF_EXECUTABLE /usr/gapps/GEOSX/thirdPartyLibs/python/linux-rhel7-x86_64-gcc@8.1.0/python@3.8.5/bin/yapf CACHE PATH "" FORCE)

To run the test, we just need to run make yapf_check. These checks are added using the same blt interface as used for uncrustify, so I assume that the CI check would be similar.

@bmhan12 - I assume that you are good person to talk to about this?

bmhan12 commented 1 year ago

@cssherman You could do this by updating thirdPartyLibs and installing yapf. Or, if the right python3 is available on the Azure Pipelines agent image, possibly install yapf there instead.

thirdPartyLibs You would need to modify the Dockerfile recipe to install pip, and then install yapf. Something like this (Ubuntu):

apt-get install python3-pip
python3 -m pip install yapf

Then rebuild the TPLs.

Would also need to update host-config for the CI run to find the installed yapf path: https://github.com/GEOSX/GEOSX/blob/71c695da3c4aa6ab37a652507ac552476d1fd812/host-configs/tpls.cmake#L85-L87

Considering the system's python is installing yapf and not our thirdPartyLibs script, the yapf path will be different (e.g. /usr/local/bin/yapf)

At this point, the yapf check should be running, assuming you have the yapf_check target added to ctest like uncrustify: https://github.com/GEOSX/GEOSX/blob/01f4b624504ec0ee915a6f457ef8cc09d054b168/src/coreComponents/CMakeLists.txt#L112-L116

If you would like to run this as a standalong CI job, like how we've separated out the style and documentation checks, you can introduce a flag to parse that runs only the yapf test.

https://github.com/GEOSX/GEOSX/blob/1446a80f6adbc174dab503ff1bbee03eab21163b/azure-pipelines.yml#L17-L26

https://github.com/GEOSX/GEOSX/blob/1446a80f6adbc174dab503ff1bbee03eab21163b/scripts/build_test_helper.sh#L57-L61

I probably forgotten something, but this is some of the changes that need to be made.

Agent Image Assuming python3 is available on the default Azure Pipelines agent image, you could also create a job that installs yapf and runs it recursively over GEOSX.

TotoGaz commented 1 year ago

If it's not a problem, could you simply add some python3 -m pip install yapf (maybe with a specific version) in the same stage as we do the doxygen test?

Nevertheless, our python management in the CI needs to be far better, and that overflows from the localized yapf question.

untereiner commented 4 months ago

@cssherman is this actually needed ?