Make Python Auxiliary CLI optional #10786

Myoldmopar · 2024-10-09T19:07:42Z

With 24.2, a new CLI category was added for auxiliary tools. This enabled packaging up EP-Launch with the official EnergyPlus release packages. This is working fine, but in our build rules, it was tied directly to the core LINK_WITH_PYTHON CMake variable. Anyone who wants to build with LINK_WITH_PYTHON will end up now getting a pip install process, which could cause issues if your Python environment is not "ready". This is especially prominent on Mac with brew install complexities around tcl-tk.

This PR adds a second CMake variable, currently called PYTHON_CLI. This is only exposed to the user if they first turn on LINK_WITH_PYTHON. Here are the options:

LINK_WITH_PYTHON OFF:
- No Python plugins, no Python packaging, nothing.
LINK_WITH_PYTHON ON, PYTHON_CLI OFF
- Back to how we were before 24.2. Python plugins are enabled, and Python core library is packaged with EnergyPlus
LINK_WITH_PYTHON ON, PYTHON_CLI ON
- How it is with 24.2, Python plugins are enabled, the python library is packaged with EnergyPlus
- In addition, during the build process, cmake also pip installs energyplus_launch, packages it up, and fixes up Tcl/Tk stuff along the way. The EnergyPlus CLI now includes the auxiliary category.

This PR simply lets plugins work without needing to also have the auxiliary stuff.

…fault

Myoldmopar

Walkthrough completed. Should be a pretty straightforward PR. I am open to discussion about the CMake variable name if anyone has really strong opinions about it.

Myoldmopar · 2024-10-09T19:08:22Z

.decent_ci-Linux.yaml

@@ -1,15 +1,15 @@
 compilers:
  - name: "gcc"
    version: "11.4"
-    cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPython_REQUIRED_VERSION:STRING=3.12.2 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.12.2/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DBUILD_PERFORMANCE_TESTS:BOOL=ON -DVALGRIND_ANALYZE_PERFORMANCE_TESTS:BOOL=ON -DENABLE_PCH:BOOL=OFF
+    cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPYTHON_CLI:BOOL=OFF -DPython_REQUIRED_VERSION:STRING=3.12.2 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.12.2/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DBUILD_PERFORMANCE_TESTS:BOOL=ON -DVALGRIND_ANALYZE_PERFORMANCE_TESTS:BOOL=ON -DENABLE_PCH:BOOL=OFF


Not necessary since it is defaulted OFF, but explicitly turn off the auxiliary Python stuff for Decent CI builds.

Myoldmopar · 2024-10-09T19:08:30Z

.decent_ci-Linux.yaml

    collect_performance_results: true
    skip_regression: true
    s3_upload_bucket: energyplus

  - name: "gcc"
    version: "11.4"
    build_type: RelWithDebInfo
-    cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPython_REQUIRED_VERSION:STRING=3.12.2 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.12.2/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_REGRESSION_TESTING:BOOL=OFF -DCOMMIT_SHA:STRING=$COMMIT_SHA -DENABLE_COVERAGE:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DENABLE_PCH:BOOL=OFF
+    cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPYTHON_CLI:BOOL=OFF -DPython_REQUIRED_VERSION:STRING=3.12.2 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.12.2/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_REGRESSION_TESTING:BOOL=OFF -DCOMMIT_SHA:STRING=$COMMIT_SHA -DENABLE_COVERAGE:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DENABLE_PCH:BOOL=OFF


Not necessary since it is defaulted OFF, but explicitly turn off the auxiliary Python stuff for Decent CI builds.

Myoldmopar · 2024-10-09T19:08:34Z

.decent_ci-Linux.yaml

@@ -23,7 +23,7 @@ compilers:
  - name: "gcc"
    version: "11.4"
    build_type: RelWithDebInfo
-    cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPython_REQUIRED_VERSION:STRING=3.12.2 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.12.2/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_REGRESSION_TESTING:BOOL=OFF -DCOMMIT_SHA:STRING=$COMMIT_SHA -DENABLE_COVERAGE:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DENABLE_PCH:BOOL=OFF
+    cmake_extra_flags: -DLINK_WITH_PYTHON:BOOL=ON -DPYTHON_CLI:BOOL=OFF -DPython_REQUIRED_VERSION:STRING=3.12.2 -DPython_ROOT_DIR:PATH=~/.pyenv/versions/3.12.2/ -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DENABLE_REGRESSION_TESTING:BOOL=OFF -DCOMMIT_SHA:STRING=$COMMIT_SHA -DENABLE_COVERAGE:BOOL=ON -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DENABLE_PCH:BOOL=OFF


Not necessary since it is defaulted OFF, but explicitly turn off the auxiliary Python stuff for Decent CI builds.

Myoldmopar · 2024-10-09T19:08:45Z

.decent_ci-Windows.yaml

@@ -2,5 +2,5 @@ compilers:
  - name: Visual Studio
    version: 16
    architecture: Win64
-    cmake_extra_flags: -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DCOMMIT_SHA=%COMMIT_SHA% -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DLINK_WITH_PYTHON=ON -DPython_EXECUTABLE:PATH=C:/Users/elee/AppData/Local/Programs/Python/Python312/python.exe
+    cmake_extra_flags: -DBUILD_FORTRAN:BOOL=ON -DBUILD_TESTING:BOOL=ON -DCOMMIT_SHA=%COMMIT_SHA% -DENABLE_GTEST_DEBUG_MODE:BOOL=OFF -DLINK_WITH_PYTHON=ON -DPYTHON_CLI:BOOL=OFF -DPython_EXECUTABLE:PATH=C:/Users/elee/AppData/Local/Programs/Python/Python312/python.exe


Not necessary since it is defaulted OFF, but explicitly turn off the auxiliary Python stuff for Decent CI builds.

Myoldmopar · 2024-10-09T19:09:03Z

.github/workflows/release_linux.yml

@@ -70,7 +70,8 @@ jobs:
      shell: bash
      run: |
        cmake -DCMAKE_BUILD_TYPE:STRING=$BUILD_TYPE \
-              -DLINK_WITH_PYTHON:BOOL=ON -DPython_REQUIRED_VERSION:STRING=${{ steps.setup-python.outputs.python-version }} \
+              -DLINK_WITH_PYTHON:BOOL=ON -DPYTHON_CLI:BOOL=ON \
+              -DPython_REQUIRED_VERSION:STRING=${{ steps.setup-python.outputs.python-version }} \


Make sure to turn PYTHON_CLI ON when doing release packages.

Myoldmopar · 2024-10-09T19:10:02Z

CMakeLists.txt

@@ -184,6 +184,7 @@ endif()
 # If LINK_WITH_PYTHON, also request the Development (libs) at the same time, to ensure consistent version between interpreter and Development
 # and ask for at least 3.8 (for the PyConfig stuff).
 if(LINK_WITH_PYTHON)
+  option(PYTHON_CLI "Build the Auxiliary CLI to Call Python Utilities" OFF)


This option is only exposed to the user once they turn LINK_WITH_PYTHON to ON.

Myoldmopar · 2024-10-09T19:10:50Z

src/EnergyPlus/CMakeLists.txt

  )
+  if (PYTHON_CLI)


Split the PythonCopyStandardLibrary.py and the Pip Install with Tcl Fixup stuff into two separate commands. This way we can protect the second one within an IF (PYTHON_CLI) condition.

If you had enabled LINK_WITH_PYTHON and PYTHON_CLI, and you disable LINK_WITH_PYTHON, this block would still be executed.

Oh, I thought this whole section was wrapped inside a LINK_WITH_PYTHON check.

This whole section is indeed wrapped inside a LINK_WITH_PYTHON condition up at line 918. This one is fine I think. But I did find a spot just below where I am creating the shortcut lnk file to EPLaunch that needed extra protection. If you confirm that it definitely needs change here, let me know.

Myoldmopar · 2024-10-09T19:11:11Z

src/EnergyPlus/CommandLineInterface.cc

@@ -234,7 +234,7 @@ Built on Platform: {}
        // bool debugCLI = false;
        app.add_flag("--debug-cli", debugCLI, "Print the result of the CLI assignments to the console and exit")->group(""); // Empty group to hide it

-#if LINK_WITH_PYTHON
+#if LINK_WITH_PYTHON && PYTHON_CLI


Only build out the auxiliary CLI if the new CLI flag is also enabled.

(side note, but shouldn't this be #ifdef ?)

That sounds fine. I changed it to ifdef and retested everything.

Myoldmopar · 2024-10-09T19:11:33Z

tst/EnergyPlus/unit/ReportCoilSelection.unit.cc

@@ -57,7 +57,7 @@
 #include <EnergyPlus/Data/EnergyPlusData.hh>
 #include <EnergyPlus/DataAirLoop.hh>
 #include <EnergyPlus/DataEnvironment.hh>
-//#include <EnergyPlus/DataHeatBalance.hh>
+// #include <EnergyPlus/DataHeatBalance.hh>


Clang format did this not me.

Myoldmopar · 2024-10-09T19:11:38Z

tst/EnergyPlus/unit/StandardRatings.unit.cc

@@ -57,7 +57,7 @@
 #include <EnergyPlus/Coils/CoilCoolingDX.hh>
 #include <EnergyPlus/CurveManager.hh>
 #include <EnergyPlus/DXCoils.hh>
-//#include <EnergyPlus/Data/EnergyPlusData.hh>
+// #include <EnergyPlus/Data/EnergyPlusData.hh>


Clang format did this not me.

jmarrec · 2024-10-10T10:53:56Z

src/EnergyPlus/CMakeLists.txt

+if(PYTHON_CLI)
+  add_compile_definitions(PYTHON_CLI)
+endif()


I'd nest it inside the above if(LINK_WITH_PYTHON) to be consistent with the options...

This could be a left over from another pass of CMake, and we shouldn't have a definition "PYTHON_CLI" is "LINK_WITH_PYTHON" is off (I did see you test with #if LINK_WITH_PYTHON && PYTHON_CLI, but this could happen if we forget the LINK_WITH_PYTHON)

test:

CMakeLists.txt

option(LINK_WITH_PYTHON "Link with Python Library for Python Plugin Builds" OFF) if(LINK_WITH_PYTHON) option(PYTHON_CLI "Build the Auxiliary CLI to Call Python Utilities" OFF) endif() if(LINK_WITH_PYTHON) message("LINK_WITH_PYTHON") endif() if(PYTHON_CLI) message("PYTHON_CLI") endif()

$ cmake -DLINK_WITH_PYTHON:BOOL=ON -DPYTHON_CLI:BOOL=ON . LINK_WITH_PYTHON PYTHON_CLI $ cmake -DLINK_WITH_PYTHON:BOOL=OFF PYTHON_CLI

This is fair. I'll protect the PYTHON_CLI a little better. As for this being leftover from a cmake pass, are you suggesting other changes need to be made here? And also - do you think I should do something other than PYTHON_CLI? I thought about AUXILIARY_CLI but it seemed that this was a Python-centric thing. PYTHON_AUXILIARY_CLI? Maybe I just shouldn't worry about the name...

Meh, all good choices really.

Got it. OK, moved the def to inside the link-with-python flag condition.

jmarrec · 2024-10-10T10:55:18Z

src/EnergyPlus/CMakeLists.txt

  )
+  if (PYTHON_CLI)


If you had enabled LINK_WITH_PYTHON and PYTHON_CLI, and you disable LINK_WITH_PYTHON, this block would still be executed.

jmarrec · 2024-10-10T10:55:43Z

src/EnergyPlus/CommandLineInterface.cc

@@ -234,7 +234,7 @@ Built on Platform: {}
        // bool debugCLI = false;
        app.add_flag("--debug-cli", debugCLI, "Print the result of the CLI assignments to the console and exit")->group(""); // Empty group to hide it

-#if LINK_WITH_PYTHON
+#if LINK_WITH_PYTHON && PYTHON_CLI


(side note, but shouldn't this be #ifdef ?)

jmarrec · 2024-10-10T11:16:12Z

Here is a suggested CMakeLists.txt based on https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html

cmake_minimum_required(VERSION 3.17)

project(Test)

option(LINK_WITH_PYTHON "Link with Python Library for Python Plugin Builds" OFF)

# This will show/hide the PYTHON_CLI option based on LINK_WITH_PYTHON
include(CMakeDependentOption)
cmake_dependent_option(PYTHON_CLI "Build the Auxiliary CLI to Call Python Utilities" OFF "LINK_WITH_PYTHON" OFF)

if(LINK_WITH_PYTHON)
  message("LINK_WITH_PYTHON")
endif()

if(PYTHON_CLI)
  message("PYTHON_CLI")
endif()

The documentation for CMakeDependentOption is kinda gross, let me explain

# cmake_dependent_option(<option> "<help_text>" <initial_value> <depends> <value_when_not_available>)

This loosely translates to:

if(LINK_WITH_PYTHON)
     option(<option> "<help_text>" <initial_value>)
else()
     set(<option> <value_when_not_available>)
endif()

There's a CACHE INTERNAL variable used to remember the old value.

Myoldmopar · 2024-10-11T14:36:36Z

Here is a suggested CMakeLists.txt

That's an interesting trick. I think it's not necessary now that we've protected the couple places we are using it, so I'm not going to add that here unless you feel really strongly about it. Thanks for pointing out the spots to add protection, I am pushing now and think it's ready.

Add PYTHON_CLI as a nested cmake variable for aux CLI, disabled by de…

16a55d9

…fault

Myoldmopar added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Oct 9, 2024

Myoldmopar assigned jmarrec Oct 9, 2024

Myoldmopar commented Oct 9, 2024

View reviewed changes

jmarrec reviewed Oct 10, 2024

View reviewed changes

Protect eplaunch link creation, and use ifdefs in the code

cac27c0

Move python-cli check to inside link-with-python check

15201bc

Myoldmopar merged commit b46af9c into develop Oct 11, 2024
6 checks passed

Myoldmopar deleted the SplitAuxiliaryBuild branch October 11, 2024 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Python Auxiliary CLI optional #10786

Make Python Auxiliary CLI optional #10786

Myoldmopar commented Oct 9, 2024

Myoldmopar left a comment

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

jmarrec Oct 10, 2024

Myoldmopar Oct 10, 2024

Myoldmopar Oct 11, 2024

Myoldmopar Oct 9, 2024

jmarrec Oct 10, 2024

Myoldmopar Oct 11, 2024

Myoldmopar Oct 9, 2024

Myoldmopar Oct 9, 2024

jmarrec Oct 10, 2024

Myoldmopar Oct 10, 2024

jmarrec Oct 10, 2024

Myoldmopar Oct 11, 2024

jmarrec Oct 10, 2024

jmarrec Oct 10, 2024

jmarrec commented Oct 10, 2024

Myoldmopar commented Oct 11, 2024

Make Python Auxiliary CLI optional #10786

Make Python Auxiliary CLI optional #10786

Conversation

Myoldmopar commented Oct 9, 2024

Myoldmopar 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

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

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

Choose a reason for hiding this comment

jmarrec commented Oct 10, 2024

Myoldmopar commented Oct 11, 2024