Skip to content

Switch to an src layout with a separate tests directory? #2581

New issue
New issue
Closed
#2598
Closed
Switch to an src layout with a separate tests directory?#2581
#2598
Assignees
namurphy
Labels
GitHub ActionsA continuous integration platform for automating tests and other tasks (see .github/ directory)Plasma Lv0 | NoviceIssues that do not require knowledge of physicsPython Lv4 | ExpertIssues that require in-depth knowledge of Pythoneffort: mediumRequiring perhaps ∼1–3 dayslintersCode linters and autoformattersmaintenanceGeneral updates to package infrastructurepackagingRelated to packaging or distributionproposalProposals that need to be decided upon before implementationrefactoring ♻️Improving an implementation without adding new functionalitystatus: needs discussionIssues & PRs that need to be discussed at a community meeting or by the Coordinating Committee
Milestone
Future

Description

@namurphy
namurphy
opened on Mar 13, 2024

Description of improvement

  1. Move the plasmapy/ directory to become src/plasmapy
  2. Move tests into a separate tests directory outside of src

The directory structure would become more like:

├── README.md
├── tox.ini
├── pyproject.toml
├── ...
├── src/
│    └── plasmapy/
│       ├── __init__.py
│       ├── formulary/
│       └── ...
├── tests/
└── tools/  ← if we decide there's a need for it?
    └── ...

(I adapted this from the Python packaging guide.)

This PR should wait until we had sufficient time to discuss this.

Motivation

The src layout is now recommended in the Python packaging guide, in the pyOpenSci packaging guide, and by the scientific Python packaging guide.

The Python Packaging Guide notes that there are real consequences of a flat layout:

  • The src layout requires installation of the project to be able to run its code, and the flat layout does not.

  • This means that the src layout involves an additional step in the development workflow of a project (typically, an editable installation is used for development and a regular installation is used for testing).

  • The src layout helps prevent accidental usage of the in-development copy of the code.

    This is relevant since the Python interpreter includes the current working directory as the first item on the import path. This means that if an import package exists in the current working directory with the same name as an installed import package, the variant from the current working directory will be used. This can lead to subtle misconfiguration of the project’s packaging tooling, which could result in files not being included in a distribution.

    The src layout helps avoid this by keeping import packages in a directory separate from the root directory of the project, ensuring that the installed copy is used.

  • The src layout helps enforce that an editable installation is only able to import files that were meant to be importable. This is especially relevant when the editable installation is implemented using a path configuration file that adds the directory to the import path.

  • The flat layout would add the other project files (eg: README.md, tox.ini) and packaging/tooling configuration files (eg: setup.py, noxfile.py) on the import path. This would make certain imports work in editable installations but not regular installations.

If others look to PlasmaPy as an example for Python packaging, having an src layout would set a good example of currently recommended practices.

Packaging the tests outside of src might have the advantage of making it easier to include tests in source distributions (sdists) but not include tests in wheels (I'm not 100% of this, though). Having wheels be small is advantageous because large package sizes can be a barrier for running on the cloud (see astropy/astropy#16152).

Some disadvantages are:

  • Switching may take between a few hours (if we're lucky) and a few days (if there are any difficult-to-fix problems that arise).
  • The new directory structure will take some time to get used to.
  • The change may disrupt all current pull requests (though if we do git mv plasmapy src/, the magic of git may handle things for us)
  • The change may disrupt current setups and shortcuts. (I'll have some editing to do in my .bashrc file, that's for sure.)
  • PlasmaPy's package structure will differ from Astropy, SunPy & most Python in Heliophysics Community packages, NumPy, Scipy, xarray, etc. There is some discussion to switch, though (like in Move astropy to src directory astropy/astropy#13678).

Implementation strategy

We would need to update pyproject.toml and MANIFEST.in for sure, which would probably be fairly straightforward. We'll probably also need to update tox.ini, mypy.ini, .pre-commit-config, and .pre-commit-search-and-replace. We might need to update some of the files in .github/workflows and docs too, though I'm not sure where.

We'll need to update the contributor guide in a bunch of places. (We could probably grep for plasmapy or plasmapy/, and possibly the :file: reStructuredText role.)

We should plan to perform a pre-release ahead of the next release to make sure everything is being handled correctly.

We should pick a time when there are relatively few PRs happening, and we would want to get this in quickly to reduce the chances of merge conflicts (though if we use git mv then we may not need to worry about this). If we decide to go this route, I'd suggest doing this sooner rather than later.

Afterwards, we'll want to update .git-blame-ignore-revs so that doing git blame will ignore these changes. (There are a bunch of other PRs we should add to this file too.)

Additional context

Related issue: #1877.

Activity

  1. namurphy
    added
    refactoring ♻️Improving an implementation without adding new functionality
    proposalProposals that need to be decided upon before implementation
    on Mar 13, 2024
  2. namurphy
    added this to the Future milestone on Mar 13, 2024
  3. namurphy
    added
    effort: mediumRequiring perhaps ∼1–3 days
    status: needs discussionIssues & PRs that need to be discussed at a community meeting or by the Coordinating Committee
    GitHub ActionsA continuous integration platform for automating tests and other tasks (see .github/ directory)
    Python Lv4 | ExpertIssues that require in-depth knowledge of Python
    Plasma Lv0 | NoviceIssues that do not require knowledge of physics
    packagingRelated to packaging or distribution
    lintersCode linters and autoformatters
    maintenanceGeneral updates to package infrastructure
    on Mar 13, 2024
  4. namurphy
    self-assigned this
    on Mar 13, 2024

  5. pheuer commented on Mar 14, 2024

    @pheuer
    pheuer
    on Mar 14, 2024
    Member

    Some disadvantages are:

    Switching may take between a few hours (if we're lucky) and a few days (if there are any difficult-to-fix problems that arise).

    Seems like a script could be written to move the files and make the relevant import statement adjustments?

    The new directory structure will take some time to get used to.

    That's true, but doesn't sound that bad to me

    The change may disrupt all current pull requests (though if we do git mv plasmapy src/, the magic of git may handle things for us)

    This sounds like the most serious issue. I don't know about the git solution: if that works, we'd need good instructions on doing that. For PRs that only touch a few files, it might be easiest to copy the changed contents into another document, restart the branch from main, then copy back in the proposed changes? That would effectively disrupt the chain of commits prior to that point. Sounds like a headache!

    The change may disrupt current setups and shortcuts. (I'll have some editing to do in my .bashrc file, that's for sure.)

    I'm unsophisticated enough that I run the tests manually most of the time, so this wouldn't bother me :P

    PlasmaPy's package structure will differ from Astropy, SunPy & most Python in Heliophysics Community packages, NumPy, Scipy, xarray, etc. There is some discussion to switch, though (like in astropy/astropy#13678).

    Sounds fine if we are following updated guidelines. At the same time, it might be prudent to wait for some of these other packages to switch prior to following along?

  6. namurphy commented on Mar 14, 2024

    @namurphy
    namurphy
    on Mar 14, 2024
    MemberAuthor

    Thank you for the feedback!

    I don't know about the git solution: if that works, we'd need good instructions on doing that.

    If git mv works its wonders, it could be that a single git pull upstream main would handle this. Otherwise...yes, we would definitely need instructions.

    Sounds fine if we are following updated guidelines. At the same time, it might be prudent to wait for some of these other packages to switch prior to following along?

    Since there's ongoing discussion (and apprehension) in Astropy about switching to an src layout (or at least a layout with a separate tests directory), I was thinking that PlasmaPy could serve as a "prototype" for making these changes. It was actually the discussion in Astropy that sparked me to raise this issue (though I've been thinking it for a while).

  7. namurphy commented on Mar 14, 2024

    @namurphy
    namurphy
    on Mar 14, 2024
    MemberAuthor

    I'm finding it interesting to read through the issue on PyPA about whether to recommend the src layout in their Python Packaging Guide (pypa/packaging.python.org#320). There were a lot of very interesting perspectives raised there. Overall it sounds like it isn't so much that there's a single overarching reason to use an src layout, but rather that there are a bunch of smaller compelling reasons why src layouts are helpful for preventing subtle packaging and testing problems.

    One thing to note is that an src layout would require us to do an editable installation of PlasmaPy before running tests directly with pytest. I've seen this noted as both a pro and a con of src layouts. It's more work to run pytest, but this makes it certain that the tests are being run against the installed version of PlasmaPy (i.e., there is parity with how users use PlasmaPy). [Then again, tox is only a few seconds longer than pytest if it doesn't have to recreate the environment, so maybe we should start recommending tox as the preferred way to run tests...but that's for another issue.]

  8. namurphy
    closed this as completedon Mar 14, 2024
  9. namurphy
    reopened this on Mar 14, 2024

  10. namurphy commented on Mar 21, 2024

    @namurphy
    namurphy
    on Mar 21, 2024
    MemberAuthor

    I just did an experiment where I entered a branch for an existing PR, and then did:

    mkdir src
git mv plasmapy src/
git pull upstream main
git commit -n -m "Pull in changes"  # -n ← skip pre-commit

    Because this worked very smoothly, I suspect that a switch to an src layout will not cause any major disruptions to ongoing pull requests. The disruption will probably be limited to requiring a git pull upstream main and for any open files to be closed and reopened at the new location.

  11. namurphy
    closed this as completedin #2598on Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

GitHub ActionsA continuous integration platform for automating tests and other tasks (see .github/ directory)Plasma Lv0 | NoviceIssues that do not require knowledge of physicsPython Lv4 | ExpertIssues that require in-depth knowledge of Pythoneffort: mediumRequiring perhaps ∼1–3 dayslintersCode linters and autoformattersmaintenanceGeneral updates to package infrastructurepackagingRelated to packaging or distributionproposalProposals that need to be decided upon before implementationrefactoring ♻️Improving an implementation without adding new functionalitystatus: needs discussionIssues & PRs that need to be discussed at a community meeting or by the Coordinating Committee

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions