Skip to content

Convert legacy setuptools use to pyproject.toml #544

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

Merged
merged 4 commits into from
May 9, 2025

Conversation

ngie-eign
Copy link
Contributor

@ngie-eign ngie-eign commented Apr 18, 2025

This change does some of the needed work to push the deepdiff project towards the new (pyproject.toml) format.

This change also adds some niceties, like:

  • Linting with flake8/mypy.
  • Testing, etc, with nox.

While here, fix some minor critical items keeping pyflakes from running cleanly on the code.

Copy link
Owner

@seperman seperman left a comment

Choose a reason for hiding this comment

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

Hi @ngie-eign
Thanks for making the PR!

Proposal: Drop Tox from our GitHub Actions and invoke tools directly

Since we already use the Actions matrix + setup-python with cache: pip keyed on pyproject.toml, running Tox in CI just creates fresh venvs and reinstalls everything (ignoring our cache) on every job. We can speed up CI and simplify the workflow by:

  1. Installing our extras once with pip
  2. Calling flake8 and pytest directly

Below is a replacement you can copy-paste into the PR:

     - name: Setup Python ${{ matrix.python-version }} on ${{ matrix.architecture }}
-      uses: actions/setup-python@v2
+      uses: actions/setup-python@v5
       with:
         python-version: ${{ matrix.python-version }}
         architecture: ${{ matrix.architecture }}
+        cache: pip
+        cache-dependency-path: pyproject.toml

-    - name: Install dependencies
-      if: matrix.python-version > 3.9
-      run: pip install -r requirements-dev.txt
-    - name: Install dependencies
-      if: matrix.python-version <= 3.9
-      run: pip install -r requirements-dev3.8.txt
+    - name: Install dev & test deps
+      run: |
+        pip install -U pip
+        pip install -e ".[cli,test]"

-    - name: Lint with flake8
-      if: matrix.python-version == 3.12
-      run: |
-        tox -e flake8 -- deepdiff --count --select=E9,F63,F7,F82 --show-source --statistics
-        tox -e flake8 -- deepdiff --count --exit-zero --max-complexity=26 --max-line-lengt=250 --statistics
+    - name: Lint with flake8
+      if: matrix.python-version == '3.12'
+      run: |
+        flake8 deepdiff \
+          --count --select=E9,F63,F7,F82 --show-source --statistics
+        flake8 deepdiff \
+          --count --exit-zero --max-complexity=26 --max-line-length=250 --statistics

-    - name: Test with pytest and get the coverage
-      if: matrix.python-version == 3.12
-      run: |
-        tox -s -- --benchmark-disable --cov-report=xml --cov=deepdiff tests/ --runslow
-    - name: Test with pytest and no coverage report
-      if: matrix.python-version != 3.12
-      run: |
-        tox -s -- --benchmark-disable tests/
+    - name: Run tests
+      run: |
+        pytest \
+          --benchmark-disable \
+          ${{ matrix.python-version == '3.12' && '--cov=deepdiff --cov-report=xml' || '' }} \
+          tests/ --runslow

Why?

  • ✅ 100% pip cache hits (no redundant installs)
  • ✅ Faster CI startup (no extra venv creation)
  • ✅ CI config is leaner and more transparent

We can still keep Tox in our dev extras for local multi-version testing (e.g. pip install -e ".[dev]" && tox -e py38,py39,…), but drop it from the CI pipeline.

@ngie-eign
Copy link
Contributor Author

ngie-eign commented May 4, 2025

Hi @ngie-eign Thanks for making the PR!

No problem! Thank you for making this OSS project!

Proposal: Drop Tox from our GitHub Actions and invoke tools directly

I think there’s a way to strike a balance between what I originally proposed and what’s proposed in the original comment while keeping tox use. Although some non-standard use is required, using some documented “tox cleverness” could avoid some of the overhead associated with creating multiple venvs.

I’m going to do a bit more digging before I propose an implementation… but I’m definitely taking your proposal into consideration.

Why?

  • ✅ 100% pip cache hits (no redundant installs)
  • ✅ Faster CI startup (no extra venv creation)
  • ✅ CI config is leaner and more transparent

We can still keep Tox in our dev extras for local multi-version testing (e.g. pip install -e ".[dev]" && tox -e py38,py39,…), but drop it from the CI pipeline.

The drawback of not relying on a single tool is that it means you need to support 2 different workflows/sources of truth for your project, which could result in either workflow being broken if the other is not touched. I deal with a system that splits CI/CD and dev workflows at $work… and let’s just say it’s a mess when either overall flow needs to be touched :/.

Using a single tool like nox or tox would streamline your workflows and codify requirements so it’s easier for folks to spin up a container locally and run the CI/CD workflow, or run it directly in their host environment, and make it easier to troubleshoot/document how development and contributing should work with your project.

Reusing venvs can also result in issues too, depending on whether or not the jobs are run serially or in parallel, so as always, some care is required when designing these workflows. Regardless, my proposed implementation is pretty basic because I have generally simple projects, so.. fair point with potential concerns over scaling.

Counter points to consider:

  1. Does this project have enough commits/contributions to warrant venv reuse in tox, at the potential cost of atomicity/determinism/complexity?
  2. Is the contributor to maintainer ratio high enough to warrant scaling/splitting up your workflow matrix in this manner?

@seperman
Copy link
Owner

seperman commented May 6, 2025

@ngie-eign Thanks for reviewing my proposal. Your points are valid. Inconsistency between test results in local and on github can potentially waste a lot of time.
What if instead of letting go of github's parallelism, we keep using github matrix, but also locally we run the github actions. Doing so means consistent workflows between the local dev and github actions. I just found out about https://github.com/nektos/act which allows local run of github actions! What do you think?

@seperman
Copy link
Owner

seperman commented May 6, 2025

Also, while we are discussing this PR, we should drop the support for python 3.8 which had an end of life of Nov 2024. That should make the github actions logic simpler.

ngie-eign added 2 commits May 6, 2025 19:21
This makes the code pyflakes clean.

Signed-off-by: Enji Cooper <[email protected]>
This change makes it possible to run tox against python 3.9-3.13 with
a supporting pyproject.toml file (forthcoming).

This also unbreaks installing the dev package under python 3.9 by loosening
the required version for numpy.

Signed-off-by:	Enji Cooper <[email protected]>
@ngie-eign
Copy link
Contributor Author

Also, while we are discussing this PR, we should drop the support for python 3.8 which had an end of life of Nov 2024. That should make the github actions logic simpler.

Slightly -- not a ton :).

Doing some more digging, it looks like nox would be a better fit for what you're looking than tox. It has a number of options which allows for native virtual environment reuse, etc.

I'll do a fixup commit with the change just to demo the difference between nox and tox, config/environment wise.

This change simplifies the build logic on the new packaging metadata
format provided with `pyproject.toml` using the flit build backend. The
setuptools build backend wasn't featureful enough to be usable.

This still doesn't fix the fact that installing `deepdiff` results in a
broken `deep` CLI command, but it at least pushes the ball towards a
world where that will be possible, someday.

Signed-off-by: Enji Cooper <[email protected]>
This change modifies the strategy used by the project to use `nox`
instead of `tox` as the former better supports virtual environment
reuse.

Signed-off-by: Enji Cooper <[email protected]>
@ngie-eign
Copy link
Contributor Author

@seperman : please look at the latest diff -- it replaces tox use with nox, which I believe achieves what you were looking for.

@seperman
Copy link
Owner

seperman commented May 9, 2025

Thanks let me check!

@seperman seperman merged commit 724938d into seperman:master May 9, 2025
@seperman
Copy link
Owner

seperman commented May 9, 2025

Thank you @ngie-eign

@ngie-eign ngie-eign deleted the pyproject_toml branch May 9, 2025 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants