Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up requirements.txt file(s) #871

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

schaubh
Copy link
Contributor

@schaubh schaubh commented Dec 10, 2024

  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

The requirements.txt had a mixture of BSK run-time python package requirements, but also requirements to build BSK that didn't belong in this file. Now, the requirements.txt file only contains python packages required to run BSK along with the examples files.

The packages required to build BSK from source are moved to requirements_dev.txt. This simplifies the BSK install instructions to use::

pip install -r requirements_dev.txt

If the build packages change, or their version requirements change, we can readily address that in this file without having to change the install instructions.

All packages to build the RST documentation are moved to the new requirements_doc.txt file. The requirements_optional.txt file is no longer needed. Only a small number of packages were left to run particular examples, and these are moved to requirements.txt.

Verification

Did a clean build and BSK compiled from scratch without issues. The CI build scripts are updated to use the requirements_dev.txt file and build across all platforms and python versions.

Documentation

The BSK Documentation files have been updated to discuss the install instructions and the instructions how to build the documentation. The mention of requirements_optional.txt have been removed.

Future work

None at this time.

@schaubh schaubh added documentation Improvements or additions to documentation ci Continuous integration build Build system or compilation enhancement labels Dec 10, 2024
@schaubh schaubh requested a review from andrewmorell December 10, 2024 14:40
@schaubh schaubh self-assigned this Dec 10, 2024
@schaubh schaubh requested a review from a team as a code owner December 10, 2024 14:40
@schaubh
Copy link
Contributor Author

schaubh commented Dec 10, 2024

@dpad , you mentioned the need to clean up requirements.txt. This branch ensures requirements.txt only contains run-time python packages. Packages related to building BSK are now moved to requirements_dev.txt, and packages related to building documentation are in requirements_doc.txt. Let me know if you have feedback on this arrangement. I think it cleans up the build instructions as the use just installs what is in requirements_dev.txt prior to building BSK or a wheel.

@schaubh schaubh marked this pull request as draft December 10, 2024 15:40
@schaubh schaubh marked this pull request as ready for review December 10, 2024 16:58
cmake version is already checked in src/CMakeList.txt
The variable required_conan_version defines the required version of conan in this file.  No need to duplicate this checking.
all build required dependencies are move to this new file
Move the documentation related packages to requirements_doc.txt, and the other optional packages are now part of the regular requirements.txt file.  The wheel build option to include additional packages is really not needed.

If using the allOptPkg flag now the documentation related packages are installed
pkg_resources is deprecated and was throwing warnings.
@schaubh schaubh force-pushed the feature/requirements_update branch from f9d58a3 to 00ab5e0 Compare December 11, 2024 16:33
Copy link
Contributor

@andrewmorell andrewmorell left a comment

Choose a reason for hiding this comment

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

Looks good! Everything built, tested, and generated documentation well on my end. Just one final place that mentions requirements_optional.txt snuck through that needs to be removed: scenarioVisualizedMonteCarlo.py line 75

- the RST document on optional packages is no longer needed
- the documentation on using `pre-commit` and `clang-format` is now moved to the coding guidelines RST file. This is a better place for this.
- update table description of the `allOptPkg` flag
- moved RST file to create BSK documentation to inside the Developer folder
- updated RST on creating the BSK documentation to discuss loading required packages from requirements_doc.txt
Users should just load all packages defined in requirements_dev.txt before building BSK.  This ensures all the build related requirements are satisfied.
This package wasn't being use by any of the BSK test or example scripts.  Further, the current dask-expr is no longer compatible with Python 3.8.  Thus, this is being removed now from the BSK requirements.
As pytest and pytest-xdist are no longer in requirements.txt, we need to install these manually in this test script.
@schaubh schaubh force-pushed the feature/requirements_update branch from 00ab5e0 to 1023c61 Compare December 12, 2024 00:30
@schaubh schaubh requested a review from andrewmorell December 12, 2024 00:30
Copy link
Contributor

@andrewmorell andrewmorell left a comment

Choose a reason for hiding this comment

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

All ready now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build system or compilation enhancement ci Continuous integration documentation Improvements or additions to documentation
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants