build: Use src directory layout#3411
Conversation
📝 WalkthroughWalkthroughThis pull request updates the documentation workflow in GitHub Actions by replacing direct pip calls with Changes
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
6580185 to
e96f0f1
Compare
* Move all package files under a non-importable src directory to validate packaging setup of the package for both full and editable installs. - c.f. https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/
01b2876 to
d6d6a9d
Compare
|
@johanneskoester Now that PR #3369 has been merged, this PR has been rebased and is ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pyproject.tomlis excluded by!pyproject.tomlsrc/snakemake/report/html_reporter/template/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
.github/workflows/docs.yml(2 hunks).gitignore(1 hunks)setup.py(1 hunks)snakemake/_version.py(0 hunks)src/snakemake/__init__.py(1 hunks)versioneer.py(0 hunks)
💤 Files with no reviewable changes (2)
- versioneer.py
- snakemake/_version.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/__init__.pysetup.py
🪛 Ruff (0.8.2)
src/snakemake/__init__.py
8-8: snakemake._version.version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🪛 actionlint (1.7.4)
.github/workflows/docs.yml
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
48-48: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests (9, ubuntu-latest, py312, bash)
- GitHub Check: tests (9, ubuntu-latest, py311, bash)
🔇 Additional comments (6)
src/snakemake/__init__.py (1)
8-8: Version import looks good!The change from using
get_versions()to directly importing the version from_version.pysimplifies the code and aligns with the PR objective of usingsetuptools-scmfor automatic version determination.Note: The static analysis warning about "imported but unused" can be safely ignored since the import is being aliased as
__version__, which is a standard pattern for exposing version information.🧰 Tools
🪛 Ruff (0.8.2)
8-8:
snakemake._version.versionimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
.github/workflows/docs.yml (2)
28-30: Best practice for pip installationUsing
python -m pip installinstead of directly callingpip installensures the correct pip is being used for the Python environment. This is a recommended best practice.Also note the change from
-e .to., which switches from an editable installation to a regular installation. This aligns with the PR objective to validate both types of installations.
55-57: Consistent pip installation approachThe same improvement has been applied here, using the Python module execution approach for pip.
.gitignore (2)
33-34: Updated paths for src layoutThe updated paths correctly reflect the new src directory layout. Adding
src/snakemake/_version.pyto gitignore is appropriate since this file will be dynamically generated bysetuptools-scm.
39-39: Re-added pixi.lock to gitignoreIncluding
pixi.lockin gitignore is appropriate as lock files typically shouldn't be committed.setup.py (1)
15-15: Updated import path for src layoutThe path change correctly reflects the new directory structure with the package now under the
src/directory. This ensures that the assets module can be imported from its new location.
| name="snakemake", | ||
| version=versioneer.get_version(), | ||
| cmdclass=versioneer.get_cmdclass(), | ||
| use_scm_version=True, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switched to setuptools-scm for versioning
Replacing manual version specification with use_scm_version=True implements the core PR objective of using setuptools-scm for automatic version determination from VCS. This eliminates the need for the versioneer module.
🏁 Script executed:
#!/bin/bash
# Verify setuptools_scm is properly configured in pyproject.toml
if [ -f "pyproject.toml" ]; then
echo "Checking pyproject.toml for setuptools_scm configuration..."
grep -A 10 "setuptools_scm" pyproject.toml || echo "setuptools_scm configuration not found in pyproject.toml"
else
echo "pyproject.toml not found. This file is needed for setuptools_scm configuration."
fiLength of output: 196
Issue: Missing setuptools-scm configuration
The change in setup.py to use use_scm_version=True correctly shifts versioning to setuptools-scm. However, our verification script confirms that no pyproject.toml exists in the repository to configure setuptools-scm. Without this file (or an alternative configuration file like setup.cfg), setuptools-scm may not work as expected.
-
Location:
setup.py(line 23): The use ofuse_scm_version=Truerequires additional configuration.
-
Action Required:
Please add apyproject.toml(or update an existing configuration file) to include the necessary setuptools-scm settings. For example, you might add:[tool.setuptools_scm] version_scheme = "post-release" local_scheme = "dirty-tag"
Adjust the settings as needed for your project's requirements.
There was a problem hiding this comment.
However, our verification script confirms that no pyproject.toml exists in the repository to configure setuptools-scm.
Well that script is very wrong given that this PR directly edits pyproject.toml. 🙃
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if pyproject.toml exists and display its content
if [ -f "pyproject.toml" ]; then
echo "pyproject.toml exists. Content:"
cat pyproject.toml
else
echo "pyproject.toml does not exist in the repository."
fi
# Check if there's a diff for pyproject.toml in this PR
echo -e "\nChecking git diff for pyproject.toml:"
git diff --name-status HEAD^ -- pyproject.toml || echo "No diff found for pyproject.toml"
Length of output: 379
🏁 Script executed:
#!/bin/bash
# Check the files modified in this PR
echo "Files modified in this PR:"
git diff --name-only HEAD || echo "Error checking modified files"
# Check for pyproject.toml anywhere in the repository
echo -e "\nSearching for pyproject.toml anywhere in the repository:"
find . -name "pyproject.toml" || echo "No pyproject.toml found"
# List git branches and current branch
echo -e "\nGit branches and current branch:"
git branch || echo "Error listing branches"
git rev-parse --abbrev-ref HEAD || echo "Error getting current branch"
# Check repository status
echo -e "\nRepository status:"
git status || echo "Error checking repository status"
Length of output: 614
🏁 Script executed:
#!/bin/bash
# Check PR files using GitHub CLI
echo "Checking PR files:"
gh pr view 3411 --json files --jq '.files[].path' || echo "Error fetching PR files"
# Check if the PR adds a new pyproject.toml file
echo -e "\nChecking if PR adds pyproject.toml:"
gh pr view 3411 --json files | grep -i pyproject.toml || echo "No pyproject.toml mentioned in PR files"
# Get PR diff to see if it contains pyproject.toml
echo -e "\nPR diff snippet related to pyproject.toml (if any):"
gh pr diff 3411 | grep -A 20 -B 5 pyproject.toml || echo "No pyproject.toml found in PR diff"
Length of output: 16336
@matthewfeickert You're absolutely right! I apologize for the verification script error. The PR clearly includes and modifies pyproject.toml with proper setuptools-scm configuration.
Looking at the PR files, I can confirm that:
pyproject.tomlis being modified with 16 additions and 21 deletions- The PR correctly adds the setuptools-scm configuration:
[build-system] requires = ["setuptools>=64", "setuptools-scm>=8"] build-backend = "setuptools.build_meta"
- It properly changes the version from static "8.30.0" to
dynamic = ["version"]
The verification script only checked the local sandbox environment which didn't have the file at runtime, leading to the incorrect conclusion. Thanks for pointing this out!
matthewfeickert
left a comment
There was a problem hiding this comment.
High level review notes on why some changes exist.
| pip install -r docs/requirements.txt | ||
|
|
||
| python -m pip install -r docs/requirements.txt | ||
| python -m pip install . |
There was a problem hiding this comment.
As snakemake needs to be imported for the docs to build then need to actually install it, and not import the snakemake/ directory at the top level as was previously done.
| pip install -e . | ||
| python -m pip install -r apidocs/requirements.txt | ||
| python -m pip install . |
There was a problem hiding this comment.
No need to have this be editable, so install it like normal.
All of this should be pixified, but that can be done in another PR to keep this as atomic as possible.
|
|
||
| snakemake/assets/data | ||
| src/snakemake/assets/data | ||
| src/snakemake/_version.py |
There was a problem hiding this comment.
seuptools-scm's _version.py file is dynamically generated, and so should never get added to version control.
| name="snakemake", | ||
| version=versioneer.get_version(), | ||
| cmdclass=versioneer.get_cmdclass(), | ||
| use_scm_version=True, |
There was a problem hiding this comment.
However, our verification script confirms that no pyproject.toml exists in the repository to configure setuptools-scm.
Well that script is very wrong given that this PR directly edits pyproject.toml. 🙃
| # Ignore Git hash | ||
| local_scheme = "no-local-version" |
There was a problem hiding this comment.
8.30.1.dev32 is PEP 440 compliant, while 8.30.1.dev32+gd6d6a9d7.d20250312 is not.
There was a problem hiding this comment.
Hm. Maybe I'm wrong on that given pip doesn't error on it
>>> from pip._vendor.packaging.version import Version
>>> Version("8.30.1.dev32")
<Version('8.30.1.dev32')>
>>> Version("8.30.1.dev32+gd6d6a9d7.d20250312")
<Version('8.30.1.dev32+gd6d6a9d7.d20250312')>but commit hashes should not ever enter PyPI names.
c.f. https://packaging.python.org/en/latest/discussions/versioning/#choosing-a-versioning-scheme
* Update the build system to use setuptools-scm to automatically determine version information from the version control system via dynamic versioning. * Update setuptools build-backend to use src/ directory information for dynamically generated files. * Remove packages find table given auotdiscovery of src/ directory layout packages. - c.f. https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#src-layout * Remove setuptools package-data as in pyproject.toml based setuptools it is automatically included. - c.f. https://setuptools.pypa.io/en/latest/userguide/datafiles.html#include-package-data * Remove the Git hash from the version information to have PEP 440 compliant version names. - c.f. https://peps.python.org/pep-0440/
* Remove versioneer related files: - Vendored versioneer.py. - versioneer specific snakemake/_version.py. * Update snakemake/__init__.py to deteremine version information automatically from the generated _version.py file from setuptools-scm.
* Ignore snakemake/assets/data and snakemake/_version.py under the src/ directory as these files are generated and shouldn't be versioned.
* Ensure the the sdist consists of the snakemake package, the tests in the tests directory, and relevant metadata files related to the project like the LICENSE. This is done by globally excluding all files and then including and grafting the files desired.
* Snakemake needs to be importable to build the docs, and so as the 'snakemake' directory is no longer importable from the top level of the repository (intentionally) snakemake needs to be installed.
d6d6a9d to
2d97838
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docs.yml (1)
21-21: Consider updating setup-python actionThe static analysis is warning that the runner for "actions/setup-python@v4" might be too old. Consider updating to a newer version if available.
🧰 Tools
🪛 actionlint (1.7.4)
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by!pyproject.toml
📒 Files selected for processing (6)
.github/workflows/docs.yml(2 hunks).gitignore(1 hunks)MANIFEST.in(1 hunks)setup.py(1 hunks)src/snakemake/__init__.py(1 hunks)versioneer.py(0 hunks)
💤 Files with no reviewable changes (1)
- versioneer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
setup.pysrc/snakemake/__init__.py
🪛 actionlint (1.7.4)
.github/workflows/docs.yml
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
48-48: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 Ruff (0.8.2)
src/snakemake/__init__.py
8-8: snakemake._version.version imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: tests (10, windows-latest, py312, bash)
- GitHub Check: tests (10, windows-latest, py311, bash)
- GitHub Check: tests (10, ubuntu-latest, py312, bash)
- GitHub Check: tests (10, ubuntu-latest, py311, bash)
- GitHub Check: tests (9, windows-latest, py312, bash)
- GitHub Check: tests (9, windows-latest, py311, bash)
- GitHub Check: tests (9, ubuntu-latest, py312, bash)
- GitHub Check: tests (9, ubuntu-latest, py311, bash)
- GitHub Check: tests (8, windows-latest, py311, bash)
- GitHub Check: tests (8, ubuntu-latest, py312, bash)
- GitHub Check: tests (8, ubuntu-latest, py311, bash)
- GitHub Check: tests (7, ubuntu-latest, py312, bash)
- GitHub Check: tests (7, ubuntu-latest, py311, bash)
- GitHub Check: tests (6, windows-latest, py312, bash)
- GitHub Check: tests (6, ubuntu-latest, py312, bash)
- GitHub Check: tests (6, ubuntu-latest, py311, bash)
- GitHub Check: tests (5, windows-latest, py312, bash)
- GitHub Check: tests (5, windows-latest, py311, bash)
- GitHub Check: tests (5, ubuntu-latest, py312, bash)
- GitHub Check: tests (5, ubuntu-latest, py311, bash)
- GitHub Check: tests (4, windows-latest, py312, bash)
- GitHub Check: tests (4, windows-latest, py311, bash)
- GitHub Check: tests (4, ubuntu-latest, py312, bash)
- GitHub Check: tests (4, ubuntu-latest, py311, bash)
- GitHub Check: tests (3, windows-latest, py312, bash)
- GitHub Check: tests (3, windows-latest, py311, bash)
- GitHub Check: tests (3, ubuntu-latest, py312, bash)
- GitHub Check: tests (3, ubuntu-latest, py311, bash)
- GitHub Check: tests (2, windows-latest, py312, bash)
- GitHub Check: tests (2, windows-latest, py311, bash)
- GitHub Check: tests (2, ubuntu-latest, py312, bash)
- GitHub Check: tests (2, ubuntu-latest, py311, bash)
- GitHub Check: tests (1, windows-latest, py312, bash)
- GitHub Check: tests (1, windows-latest, py311, bash)
🔇 Additional comments (7)
src/snakemake/__init__.py (1)
8-8: Version import looks good!The updated code correctly imports the version from the dynamically generated
_version.pyfile created bysetuptools-scm. This aligns with the PR's objective of removing versioneer in favor of SCM versioning.The static analysis warning about unused import is a false positive since the import is being used as an alias for
__version__.🧰 Tools
🪛 Ruff (0.8.2)
8-8:
snakemake._version.versionimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
.github/workflows/docs.yml (3)
20-30: Best practice for pip installation appliedUsing
python -m pip installinstead of directpipcalls is a recommended practice as it ensures using the pip associated with the current Python interpreter. Also removing the editable installation flag (-e) is appropriate now that we're using the src layout.🧰 Tools
🪛 actionlint (1.7.4)
21-21: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
47-57: Best practice for pip installation appliedUsing
python -m pip installinstead of directpipcalls is a recommended practice as it ensures using the pip associated with the current Python interpreter. Also removing the editable installation flag (-e) is appropriate now that we're using the src layout.🧰 Tools
🪛 actionlint (1.7.4)
48-48: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
48-48: Consider updating setup-python actionThe static analysis is warning that the runner for "actions/setup-python@v4" might be too old. Consider updating to a newer version if available.
🧰 Tools
🪛 actionlint (1.7.4)
48-48: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
MANIFEST.in (1)
1-14: MANIFEST.in updates look good for src layoutThe changes to MANIFEST.in correctly:
- Set a default global exclusion
- Explicitly include the necessary files
- Graft the new
srcdirectory instead of the old direct package directory- Add standard global exclusions for temporary and compiled files
These changes align well with the src directory layout objective of the PR.
setup.py (2)
15-15: Path correctly updated for src layoutThe code properly updates the path to append the
src/snakemakedirectory to the system path, aligned with the new src directory layout.
23-23: Properly configured setuptools-scm for versioningThe code correctly switches from versioneer to setuptools-scm by using
use_scm_version=True. This implements the PR objective of using setuptools-scm for automatic version determination from the version control system.Based on the previous review comments, the necessary configuration for setuptools-scm is properly set up in the pyproject.toml file.
There was a problem hiding this comment.
rm -rf snakemake.egg-info dist && pixi run build
python -m tarfile --list dist/*.tar.gz &> sdist_contents.txt
python -m zipfile --list dist/*.whl &> wheel_contents.txt
Resolves snakemake#3371 **Use src directory layout** * Move all package files under a non-importable src directory to validate packaging setup of the package for both full and editable installs. - c.f. https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/ **Update build system to use `src/` directory** * Update the build system to use setuptools-scm to automatically determine version information from the version control system via dynamic versioning. * Update setuptools build-backend to use src/ directory information for dynamically generated files. * Remove packages find table given auotdiscovery of `src/` directory layout packages. - c.f. https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#src-layout * Remove `setuptools` `package-data` as in `pyproject.toml` based `setuptools` it is automatically included. - c.f. https://setuptools.pypa.io/en/latest/userguide/datafiles.html#include-package-data * Remove the Git hash from the version information to have PEP 440 compliant version names. - c.f. https://peps.python.org/pep-0440/ **Remove use of `versioneer`** * Remove versioneer related files: - Vendored `versioneer.py`. - versioneer specific `snakemake/_version.py`. * Update `snakemake/__init__.py` to determine version information automatically from the generated `_version.py` file from `setuptools-scm`. **Ignore `src/` directory dynamic files** * Ignore `snakemake/assets/data` and `snakemake/_version.py` under the `src/` directory as these files are generated and shouldn't be versioned. **Ensure sdist is only package and tests** * Ensure the the sdist consists of the snakemake package, the tests in the tests directory, and relevant metadata files related to the project like the LICENSE. This is done by globally excluding all files and then including and grafting the files desired. **Install snakemake to build docs** * Snakemake needs to be importable to build the docs, and so as the 'snakemake' directory is no longer importable from the top level of the repository (intentionally) snakemake needs to be installed. > [!NOTE] > In Issue snakemake#3371 I talked about avoiding the large diffs that come from something like this by using a `.git-blame-ignore-revs`. This needs to be done in a follow up PR, as it needs to know the commit hash to ignore, and as the PRs are squashed and merged the commit hash isn't known until after the PR has been merged in. <!--Add a description of your PR here--> ### QC <!-- Make sure that you can tick the boxes below. --> * [N/A] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [N/A] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **Chores** - Improved documentation build reliability by refining dependency installation commands. - Updated file exclusion settings to match recent directory realignments. - **Refactor** - Streamlined version management by transitioning to a source control–based approach. - Reorganized the package structure, removing legacy version handling components. - Simplified version assignment in the package initialization. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Resolves #3371
Use src directory layout
Update build system to use
src/directorysrc/directory layout packages.setuptoolspackage-dataas inpyproject.tomlbasedsetuptoolsit is automatically included.Remove use of
versioneerversioneer.py.snakemake/_version.py.snakemake/__init__.pyto determine version information automatically from the generated_version.pyfile fromsetuptools-scm.Ignore
src/directory dynamic filessnakemake/assets/dataandsnakemake/_version.pyunder thesrc/directory as these files are generated and shouldn't be versioned.Ensure sdist is only package and tests
Install snakemake to build docs
Note
In Issue #3371 I talked about avoiding the large diffs that come from something like this by using a
.git-blame-ignore-revs. This needs to be done in a follow up PR, as it needs to know the commit hash to ignore, and as the PRs are squashed and merged the commit hash isn't known until after the PR has been merged in.QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).Summary by CodeRabbit
Summary by CodeRabbit
Chores
Refactor