Make pip install run only once instead of everytime CMake reconfigures #5266

jmarrec · 2024-10-03T10:17:19Z

Pull request overview

Make pip install run only once. This takes 15s to a minute (depending on pip cache and internet connection) and would run everytime cmake reconfigures, which annoyed the crap out of me.

@kbenne please review.

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

jmarrec · 2024-10-03T10:18:04Z

python/engine/CMakeLists.txt

+# Add a command to pip install the requirements into the E+ folder and generate a stamp file
+# Make it depend on the requirements.txt, so that it reruns when it changes
+add_custom_command(
+  OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/pip_install_done.stamp"
+  COMMAND ${CMAKE_COMMAND} -E touch "${CMAKE_CURRENT_BINARY_DIR}/pip_install_done.stamp"
+  COMMAND ${CMAKE_COMMAND} -E env --unset=PIP_REQUIRE_VIRTUALENV ${Python_EXECUTABLE}
+    -m pip install --target=${ENERGYPLUS_DIR}/python_lib --upgrade -r ${PROJECT_SOURCE_DIR}/python/requirements.txt
+  DEPENDS ${PROJECT_SOURCE_DIR}/python/requirements.txt
+)
+
+# And a target that calls the above command. It is NOT called by default
+add_custom_target(pip_install
+  DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/pip_install_done.stamp"
+)
+
+if(BUILD_CLI)
+  # When you build CLI, we want the pip_install to run, so add it as a dependency
+  # which will ensure it's run (beforehand, at **build** not configure time)
+  add_dependencies(pythonengine pip_install)
+endif()


This is the gist, and it's commented enough that I don't need to say anything else.

I have tested that this works, and that changing requirements.txt triggers another pip install correctly.

It's pretty much the same as

OpenStudio/ProjectMacros.cmake

Lines 153 to 180 in ba1b94b

if(NOT TARGET ${PARENT_TARGET}_GeneratedHeaders)

if ("${NAME}" STREQUAL "OpenStudioUtilitiesCore")

# Workaround appending GenerateIddFactoryRun to GeneratedHeaders, so we ensure that the custom command below actually is called AFTER

# GenerateIddFactoryRun has been called

list(APPEND GeneratedHeaders "GenerateIddFactoryRun")

endif()

# Add a command to generate the generated headers discovered at this point.

add_custom_command(

OUTPUT "${PROJECT_BINARY_DIR}/${PARENT_TARGET}_HeadersGenerated_done.stamp"

COMMAND ${CMAKE_COMMAND} -E touch "${PROJECT_BINARY_DIR}/${PARENT_TARGET}_HeadersGenerated_done.stamp"

DEPENDS ${GeneratedHeaders}

)

# And a target that calls the above command

add_custom_target(${PARENT_TARGET}_GeneratedHeaders

SOURCES "${PROJECT_BINARY_DIR}/${PARENT_TARGET}_HeadersGenerated_done.stamp"

)

# Now we say that our PARENT_TARGET depends on this new GeneratedHeaders

# target. This is where the magic happens. By making both the parent

# and this *_swig.cxx files below rely on this new target we force all

# of the generated files to be generated before either the

# PARENT_TARGET is built or the cxx files are generated. This solves the problems with

# parallel builds trying to generate the same file multiple times while still

# allowing files to compile in parallel

add_dependencies(${PARENT_TARGET} ${PARENT_TARGET}_GeneratedHeaders)

endif()

excepts I use DEPENDS instead of SOURCES in add_custom_target, because I think that's the right one.

DEPENDS
Reference files and outputs of custom commands created with add_custom_command() command calls in the same directory (CMakeLists.txt file). They will be brought up to date when the target is built.

SOURCES
Specify additional source files to be included in the custom target. Specified source files will be added to IDE project files for convenience in editing even if they have no build rules.

I think I've setup similar build rules with a custom target combined with a custom command. Each time I think I go back to the documentation looking for a way to do it with a single custom target, as I did again today, but in the end I think what you've done is the correct way. The add_custom_target command is always out of date, so we have to do this in two steps.

jmarrec · 2024-10-03T10:18:24Z

CMakeLists.txt


    execute_process(COMMAND ${CMAKE_COMMAND} -E remove_directory "${PROJECT_BINARY_DIR}/${ENERGYPLUS_PATH}")
-
+    file(REMOVE "${PROJECT_BINARY_DIR}/python/engine/pip_install_done.stamp")


When we change E+ directory (update), remove the .stamp file to trigger another install

…run everytime cmake reconfigures

kbenne · 2024-10-03T14:32:06Z

python/engine/CMakeLists.txt

+# Add a command to pip install the requirements into the E+ folder and generate a stamp file
+# Make it depend on the requirements.txt, so that it reruns when it changes
+add_custom_command(
+  OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/pip_install_done.stamp"
+  COMMAND ${CMAKE_COMMAND} -E touch "${CMAKE_CURRENT_BINARY_DIR}/pip_install_done.stamp"
+  COMMAND ${CMAKE_COMMAND} -E env --unset=PIP_REQUIRE_VIRTUALENV ${Python_EXECUTABLE}
+    -m pip install --target=${ENERGYPLUS_DIR}/python_lib --upgrade -r ${PROJECT_SOURCE_DIR}/python/requirements.txt
+  DEPENDS ${PROJECT_SOURCE_DIR}/python/requirements.txt
+)
+
+# And a target that calls the above command. It is NOT called by default
+add_custom_target(pip_install
+  DEPENDS "${CMAKE_CURRENT_BINARY_DIR}/pip_install_done.stamp"
+)
+
+if(BUILD_CLI)
+  # When you build CLI, we want the pip_install to run, so add it as a dependency
+  # which will ensure it's run (beforehand, at **build** not configure time)
+  add_dependencies(pythonengine pip_install)
+endif()


I think I've setup similar build rules with a custom target combined with a custom command. Each time I think I go back to the documentation looking for a way to do it with a single custom target, as I did again today, but in the end I think what you've done is the correct way. The add_custom_target command is always out of date, so we have to do this in two steps.

jmarrec added Developer Issue component - Python bindings labels Oct 3, 2024

jmarrec requested a review from kbenne October 3, 2024 10:17

jmarrec self-assigned this Oct 3, 2024

jmarrec commented Oct 3, 2024

View reviewed changes

jmarrec added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Oct 3, 2024

Make pip install run only once. This takes 15s to a minute and would …

463dca6

…run everytime cmake reconfigures

jmarrec force-pushed the pip_install_once branch from e2dc20c to 463dca6 Compare October 3, 2024 10:22

jmarrec changed the title ~~Make pip install run only once. This takes 30s to a minute and would …~~ Make pip install run only once instead of everytime CMake reconfigures Oct 3, 2024

kbenne approved these changes Oct 3, 2024

View reviewed changes

jmarrec merged commit 5e2016b into v24.2.0-IOFreeze Oct 4, 2024

jmarrec deleted the pip_install_once branch October 4, 2024 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make pip install run only once instead of everytime CMake reconfigures #5266

Make pip install run only once instead of everytime CMake reconfigures #5266

Uh oh!

jmarrec commented Oct 3, 2024

Uh oh!

jmarrec Oct 3, 2024

Uh oh!

jmarrec Oct 3, 2024

Uh oh!

kbenne Oct 3, 2024

Uh oh!

jmarrec Oct 3, 2024

Uh oh!

kbenne Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if(NOT TARGET ${PARENT_TARGET}_GeneratedHeaders)
	if ("${NAME}" STREQUAL "OpenStudioUtilitiesCore")
	# Workaround appending GenerateIddFactoryRun to GeneratedHeaders, so we ensure that the custom command below actually is called AFTER
	# GenerateIddFactoryRun has been called
	list(APPEND GeneratedHeaders "GenerateIddFactoryRun")
	endif()

	# Add a command to generate the generated headers discovered at this point.
	add_custom_command(
	OUTPUT "${PROJECT_BINARY_DIR}/${PARENT_TARGET}_HeadersGenerated_done.stamp"
	COMMAND ${CMAKE_COMMAND} -E touch "${PROJECT_BINARY_DIR}/${PARENT_TARGET}_HeadersGenerated_done.stamp"
	DEPENDS ${GeneratedHeaders}
	)

	# And a target that calls the above command
	add_custom_target(${PARENT_TARGET}_GeneratedHeaders
	SOURCES "${PROJECT_BINARY_DIR}/${PARENT_TARGET}_HeadersGenerated_done.stamp"
	)

	# Now we say that our PARENT_TARGET depends on this new GeneratedHeaders
	# target. This is where the magic happens. By making both the parent
	# and this *_swig.cxx files below rely on this new target we force all
	# of the generated files to be generated before either the
	# PARENT_TARGET is built or the cxx files are generated. This solves the problems with
	# parallel builds trying to generate the same file multiple times while still
	# allowing files to compile in parallel
	add_dependencies(${PARENT_TARGET} ${PARENT_TARGET}_GeneratedHeaders)
	endif()


		execute_process(COMMAND ${CMAKE_COMMAND} -E remove_directory "${PROJECT_BINARY_DIR}/${ENERGYPLUS_PATH}")

		file(REMOVE "${PROJECT_BINARY_DIR}/python/engine/pip_install_done.stamp")

Make pip install run only once instead of everytime CMake reconfigures #5266

Make pip install run only once instead of everytime CMake reconfigures #5266

Uh oh!

Conversation

jmarrec commented Oct 3, 2024

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

jmarrec Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

jmarrec Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

kbenne Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

jmarrec Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

kbenne Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants