Skip to content

#45 Migrate to a pyproject.toml file and add wheel builds. #47

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

rndubs
Copy link

@rndubs rndubs commented Jul 14, 2025

Closes #45 #46 .

Key changes to highlight:

  1. Migration from setup.py/setup.cfg to pyproject.toml (retained setup.cfg for flake8 section, which doesn't support toml out of the box)
  2. Switched from versioneer to setuptools-scm for dynamic versioning
  3. Updated Python version support (dropped 3.7/3.8, added 3.11/3.12/3.13)
  4. Updated GitHub Actions and CI/CD
  5. Updated pre-commit hooks and development tools
  6. Updated MANIFEST.in with exclusions for setuptools-scm sdist builds
  7. Consolidated tool configurations into pyproject.toml when possible
  8. Removed versioneering files (versioneer.py, _version.py)

How I've Tested It

I've run the pre-commit tests locally:

% pre-commit run --all-files
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check for added large files..............................................Passed
black....................................................................Passed
flake8...................................................................Passed
isort....................................................................Passed
autoflake................................................................Passed

and ensured that the builds include all necessary files by building and then unpacking:

python3 -m build --wheel --sdist --outdir dist/

unpack wheel:

python3 -m zipfile -l dist/logical_unification-0.4.7.dev8-py3-none-any.whl
File Name                                             Modified             Size
logical_unification-0.4.7.dev8.dist-info/licenses/LICENSE.txt 2025-07-14 01:52:36         1536
unification/__init__.py                        2025-07-11 23:39:58          207
unification/core.py                            2025-07-14 01:33:40         7114
unification/dispatch.py                        2025-07-11 22:52:26          140
unification/match.py                           2025-07-11 22:52:26         3185
unification/more.py                            2025-07-11 22:52:26         2600
unification/utils.py                           2025-07-11 22:52:26         2503
unification/variable.py                        2025-07-11 22:52:26         2647
logical_unification-0.4.7.dev8.dist-info/METADATA 2025-07-14 01:52:36        15342
logical_unification-0.4.7.dev8.dist-info/WHEEL 2025-07-14 01:52:36           91
logical_unification-0.4.7.dev8.dist-info/top_level.txt 2025-07-14 01:52:34           12
logical_unification-0.4.7.dev8.dist-info/RECORD 2025-07-14 01:52:36         1029

and the sdist:

python3 -m tarfile -l dist/logical_unification-0.4.7.dev8.tar.gz
logical_unification-0.4.7.dev8+dirty/
logical_unification-0.4.7.dev8+dirty/LICENSE.txt
logical_unification-0.4.7.dev8+dirty/MANIFEST.in
logical_unification-0.4.7.dev8+dirty/PKG-INFO
logical_unification-0.4.7.dev8+dirty/README.md
logical_unification-0.4.7.dev8+dirty/examples/
logical_unification-0.4.7.dev8+dirty/examples/account.py
logical_unification-0.4.7.dev8+dirty/logical_unification.egg-info/
logical_unification-0.4.7.dev8+dirty/logical_unification.egg-info/PKG-INFO
logical_unification-0.4.7.dev8+dirty/logical_unification.egg-info/SOURCES.txt
logical_unification-0.4.7.dev8+dirty/logical_unification.egg-info/dependency_links.txt
logical_unification-0.4.7.dev8+dirty/logical_unification.egg-info/requires.txt
logical_unification-0.4.7.dev8+dirty/logical_unification.egg-info/top_level.txt
logical_unification-0.4.7.dev8+dirty/pyproject.toml
logical_unification-0.4.7.dev8+dirty/setup.cfg
logical_unification-0.4.7.dev8+dirty/unification/
logical_unification-0.4.7.dev8+dirty/unification/__init__.py
logical_unification-0.4.7.dev8+dirty/unification/core.py
logical_unification-0.4.7.dev8+dirty/unification/dispatch.py
logical_unification-0.4.7.dev8+dirty/unification/match.py
logical_unification-0.4.7.dev8+dirty/unification/more.py
logical_unification-0.4.7.dev8+dirty/unification/utils.py
logical_unification-0.4.7.dev8+dirty/unification/variable.py

@rndubs
Copy link
Author

rndubs commented Jul 14, 2025

@brandonwillard , this PR should be ready for a look.

brandonwillard
brandonwillard previously approved these changes Jul 14, 2025
Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Thanks, again!

@brandonwillard
Copy link
Member

Agh, looks like PyPy isn't too happy about something.

@rndubs
Copy link
Author

rndubs commented Jul 14, 2025

Alright I've pushed up a change, but I haven't configured pypy for github actions before, so it might still be broken.

@rndubs
Copy link
Author

rndubs commented Jul 21, 2025

@brandonwillard , the actions for the full python interpreter matrix are now passing. This should be ready for another review.

After inspecting the logs for the previous action failures for 3.12 and 3.13, I was able to track down the issue to how CPython is calculating recursion limits. It seems that in CPython 3.12, the sys.getrecursionlimit() function changed in a way that would allow the unit tests for tests/test_benchmarks.py to complete without raising the expected recursion error. Here is the corresponding snippet from the release notes:

sys.setrecursionlimit() and sys.getrecursionlimit(). The recursion limit now applies only to Python code. Builtin functions do not use the recursion limit, but are protected by a different mechanism that prevents recursion from causing a virtual machine crash.

Full release notes: https://docs.python.org/3/whatsnew/3.12.html#sys

Since the unit test was raising a RecursionError off of the == operation (a builtin), then CPython 3.12 and 3.13 were not raising the RecursionError as the test was expecting.

I changed the test that was checking for a RecursionError since unification isn't explicitly raising or checking for that error anywhere, and I got Claude to write a few more tests to add coverage to the recursive data structures features. Let me know what you think.

@brandonwillard
Copy link
Member

brandonwillard commented Jul 28, 2025

@brandonwillard , the actions for the full python interpreter matrix are now passing. This should be ready for another review.

After inspecting the logs for the previous action failures for 3.12 and 3.13, I was able to track down the issue to how CPython is calculating recursion limits. It seems that in CPython 3.12, the sys.getrecursionlimit() function changed in a way that would allow the unit tests for tests/test_benchmarks.py to complete without raising the expected recursion error. Here is the corresponding snippet from the release notes:

sys.setrecursionlimit() and sys.getrecursionlimit(). The recursion limit now applies only to Python code. Builtin functions do not use the recursion limit, but are protected by a different mechanism that prevents recursion from causing a virtual machine crash.

Full release notes: https://docs.python.org/3/whatsnew/3.12.html#sys

Since the unit test was raising a RecursionError off of the == operation (a builtin), then CPython 3.12 and 3.13 were not raising the RecursionError as the test was expecting.

I changed the test that was checking for a RecursionError since unification isn't explicitly raising or checking for that error anywhere, and I got Claude to write a few more tests to add coverage to the recursive data structures features. Let me know what you think.

Let's just add skips for those versions. I'm not sure that the new tests are particularly helpful for that test/situation, and we don't need to complicate things, because we already know that the relevant code works (or worked for older Python versions, at least).

@rndubs
Copy link
Author

rndubs commented Jul 31, 2025

@brandonwillard , the actions for the full python interpreter matrix are now passing. This should be ready for another review.
After inspecting the logs for the previous action failures for 3.12 and 3.13, I was able to track down the issue to how CPython is calculating recursion limits. It seems that in CPython 3.12, the sys.getrecursionlimit() function changed in a way that would allow the unit tests for tests/test_benchmarks.py to complete without raising the expected recursion error. Here is the corresponding snippet from the release notes:

sys.setrecursionlimit() and sys.getrecursionlimit(). The recursion limit now applies only to Python code. Builtin functions do not use the recursion limit, but are protected by a different mechanism that prevents recursion from causing a virtual machine crash.

Full release notes: https://docs.python.org/3/whatsnew/3.12.html#sys
Since the unit test was raising a RecursionError off of the == operation (a builtin), then CPython 3.12 and 3.13 were not raising the RecursionError as the test was expecting.
I changed the test that was checking for a RecursionError since unification isn't explicitly raising or checking for that error anywhere, and I got Claude to write a few more tests to add coverage to the recursive data structures features. Let me know what you think.

Let's just add skips for those versions. I'm not sure that the new tests are particularly helpful for that test/situation, and we don't need to complicate things, because we already know that the relevant code works (or worked for older Python versions, at least).

I have reverted the test changes and added a pytest marker that skips CPython 3.12+ for the recursion tests.

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.

Migrate the setup.py packaging file to a PEP517 compliant pyproject.toml file
2 participants