GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Migrate to pyproject.toml by nirosys · Pull Request #379 · amazon-ion/ion-python

Conversation

@nirosys

@nirosys nirosys commented Oct 19, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Issue #, if available: #305

Description of changes:
This PR migrates ion-python away from a setup.py/install.py based project to a PEP 517 compliant build system. By updating the build system we are able to adopt better tooling through the formalized nature of PEP 517 projects, and how tooling can integrate with the build tree.

One tool that we can use is a PEP 517 build system called py-build-cmake which allows us use CMake for building our C extension. By using CMake, we're able to place ion-c into the build tree and include it as a subdirectory, letting py-build-cmake handle the python build flags.

A side effect of this, is that the code layout now splits the C module into src/ and the python code into src-python/.

Building the Package

Building the extension is now a simple:

$ python -m build
* Creating isolated environment: venv+pip...
* Installing packages in isolated environment:
  - py-build-cmake~=0.1.8
* Getting build dependencies for sdist...
...
* Building wheel...
...
[100%] Built target _ioncmodule
-- Installing: /var/folders/ld/sktjjrk132z_f7_95g908dxr0000gr/T/tmp4ekl7bjs/staging/amazon/_ioncmodule.so
Successfully built amazon_ion-0.12.0.tar.gz and amazon_ion-0.12.0-cp312-cp312-macosx_13_0_x86_64.whl
(venv) glitch@147dda5e5395 ~/C/ion-python>

This will build both the sdist (source distribution) and a binary wheel for the current platform & python version. This can be used to ensure the module compiles, but does not install the package. By default everything is built in isolation, using a virtual environment and pip, everything (including the C module) is built in a temporary directory.

C Extension Changes

Prior to this PR when installing the package the C extension would be present under amazon.ion.ionc. Due to a limitation in py-build-cmake (at the time of implementation) the C extension is now under top-level package, and has been renamed _ioncmodule. This is reflected in the simpleion module when testing for the extension's existance.

Dependencies

Prior to pyproject.toml, it was common practice to put dependencies into a text file called requirements.txt. With PEP 517 dependency management has been formalized, along with other metadata that previously could be provided differently depending on which build tools you were actually using.

In pyproject.toml we have a section [project] which includes a dependencies field, containing all of the dependencies needed for the package to work. We also have some optional dependencies that we can install for benchmarking, and/or unit testing. Unit testing and benchmarking are not something that most users would need when installing ion-python, so these dependencies are kept separate in the [project.optional-dependencies] section. Each list in this section is a named set of dependencies or extras that can be installed, but are not installed by default.

pip supports these optional dependencies through PEP 517, and they can be installed with pip using a command like:

$ python -m pip install -e '.[test]'

The above command would install the package in editable mode, which only installs the dependencies. Since we reference test, it includes the optional dependencies named test with the required dependencies. This allows us to keep benchmarking dependencies (like ujson, rapidjson, etc) separate so that our users do not need to install them unless they want to do benchmarking. Likewise, if the user isn't contributing or changing ion-python, there's no need for specific versions of venv and tox, required by ion-python.

Configuration

All of the 3rd party tooling configuration (eg. tox, pytest, etc) has been moved into the pyproject.toml.

Additional Changes

In addition to the pyproject.toml migration, I added in a new function for the C extension. Ion C has had a version API since Oct 2023, that can return the version and revision. I've added a new method within the _ioncmodule package that exposes this functionality:

>>> import amazon._ioncmodule
>>> amazon._ioncmodule.ionc_version()
'v1.1.3 (rev: d61c09a)'
>>>

This should make it easier to determine what Ion C fixes/bugs/etc are present in a given ion-python installation.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
nirosys added 17 commits January 15, 2025 14:26
@nirosys
Migrate to pyproject.toml, and convert the build to use py-build-cmake
7eb62dd
@nirosys
Switch to pkgutil.extend_path (revisit PEP420, this may be unneeded),…
c5c5590
… and update paths for ionc headers in extension
@nirosys
Remove install.py; Update ion-c to latest (fixes sub-directory builds)
f143a42
@nirosys
Update benchmark-cli to use new python code path
bff1587
@nirosys
Add new pyproject.toml and CMakeLists
36eeb75
@nirosys
Adjust IONC_BUILD_TESTS override
f70105d
@nirosys
Update main github actions workflow
41b60fe
@nirosys
Install build module
a358f91
@nirosys
Lower pyparsing version to support 3.8
f69ecf7
@nirosys
Further migration to pyproject.toml; move 3rd party configs into pypr…
fd2ff2f
…oject, update release workflow, and address max-int digits issue for test vectors
@nirosys
Update main workflow to include test dependencies when installing
d33a4a4
@nirosys
Update extension to link against decNumber as a static lib
56cf105
@nirosys
Update extension name, changed to _ioncmodule and is now at the top-l…
480350e
…evel
@nirosys
Fix version, it got lost in the rebase
eb51696
@nirosys
Remove requirements files; add py-build-cmake cache to gitignore
12427ad
@nirosys
Add ionc_version method to return the version of Ion C used in the ex…
60ab03a
…tension
@nirosys
Quick update pass for C Extension documentation
e6e16a2
@nirosys
nirosys force-pushed the rgiliam/build-rework branch from 635da05 to e6e16a2 Compare January 16, 2025 00:44
@nirosys

nirosys commented Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Windows build issue:

LINK : fatal error LNK1181: cannot open input file 'm.lib' [C:\Users\runneradmin\AppData\Local\Temp\build-via-sdist-rkldgbwp\amazon_ion-0.13.0\.py-build-cmake_cache\pp310-pypy310_pp73-win_amd64\_ioncmodule.vcxproj]

and I missed the performance regression workflow:

ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'requirements.txt'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nirosys added 4 commits January 16, 2025 13:09
@nirosys
Remove libm from linked libraries for windows build
dddd696
@nirosys
Update regression test to use pyproject for PR source (will need to b…
1d5923b
…e updated to do the same for baseline once merged)
@nirosys
Update package cache to only install dependencies, not our package
2434835
@nirosys
Update benchmark cli's path for PR content to account for updated layout
49ca0eb
@nirosys
nirosys marked this pull request as ready for review January 17, 2025 23:36
@nirosys
nirosys requested a review from rmarrowstone January 17, 2025 23:48
@nirosys
Update virtualenv version to match HEAD's dependabot update
070015a
rmarrowstone
rmarrowstone approved these changes Jan 19, 2025
View reviewed changes

@rmarrowstone rmarrowstone left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Awesome!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .github/workflows/main.yml Outdated
- run: pip install --upgrade setuptools
- run: pip install -r requirements.txt
- run: pip install -e .
- run: python -m pip install build virtualenv

@rmarrowstone rmarrowstone Jan 19, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why does this action also install virtualenv but the test-windows does not?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@nirosys nirosys Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah good catch, ty. I'll remove that, we're not using virtualenv in the workflows.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread .github/workflows/performance-regression.yml
specs: '{command:read,format:ion_text} {command:write,format:ion_text} {command:read,format:ion_binary} {command:write,format:ion_binary}'
test_data_id: 'generated-test-data'
run_cli: 'python amazon/ionbenchmark/ion_benchmark_cli.py'
run_cli_new: 'python src-python/amazon/ionbenchmark/ion_benchmark_cli.py'

@rmarrowstone rmarrowstone Jan 19, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should we just replace run_cli instead of run_cli_new?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@nirosys nirosys Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The new run_cli_new is temporary, to allow this PR to run the perf regression workflow. Since it pulls down the current HEAD which has the pre-PR layout, I need to run the benchmark CLI from that revision with run_cli.

Once this PR is merged, then I can follow up with a PR to replace run_cli with run_cli_new, so that the perf regression workflow continues working using the src-python layout for everything.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rmarrowstone rmarrowstone Jan 22, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread C_EXTENSION.md Outdated
compile setup import ionc module

The `amazon.ion.simpleion` module then makes use of this extension when it is available to provide more efficient
Ion parsing. Importing `amazon._ioncmodule` directly can determine if it is available, however simpleion also provides

@rmarrowstone rmarrowstone Jan 19, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

reading and writing, loading and dumping, not just parsing is all i mean.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nirosys reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread pyproject.toml
test = [
"pytest==8.3.3"
]
benchmarking = [

@rmarrowstone rmarrowstone Jan 19, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🙌

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/CMakeLists.txt Outdated
COMPONENT python_module
DESTINATION ${PY_BUILD_CMAKE_MODULE_NAME})

# install(FILES _ioncmodule.pyi

@rmarrowstone rmarrowstone Jan 19, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

delete?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@nirosys nirosys Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah! Yes, I'll get rid of that and follow up. There is a way to export python stubs for extensions to allow IDEs to present function and type information for an extension's methods. I left that as a reminder, but will follow up with it after this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rmarrowstone rmarrowstone Jan 22, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That would be neat.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/ioncmodule.c
return exception;
}

PyObject *ionc_version() {

@rmarrowstone rmarrowstone Jan 19, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nice.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
nirosys added 2 commits January 22, 2025 14:56
@nirosys
Remove unneeded virtualenv install; expand wording on C ext use; remo…
0ce40af
…ve commented cmake code
@nirosys
Address actionlint issues
3fdf19b
@nirosys
nirosys merged commit 85ee6ab into amazon-ion:master Jan 22, 2025
@nirosys nirosys mentioned this pull request Jan 23, 2025
Follow-up PR to finish updating the performance regression workflow #401
Merged
@jobarr-amzn jobarr-amzn mentioned this pull request May 21, 2025
Update development instructions #412
Merged
@nirosys nirosys mentioned this pull request Jul 24, 2025
DEPRECATION: amazon.ion is being installed using the legacy 'setup.py install' method #305
Closed
@NovemberZulu NovemberZulu mentioned this pull request Jun 25, 2026
Minimize amount of dependencies #443
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@rmarrowstone rmarrowstone rmarrowstone approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@nirosys @rmarrowstone
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.