Skip to content
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

Replace toml with tomllib & tomli #857

Merged
merged 32 commits into from
Sep 9, 2024
Merged

Replace toml with tomllib & tomli #857

merged 32 commits into from
Sep 9, 2024

Conversation

maliasadi
Copy link
Member

@maliasadi maliasadi commented Aug 16, 2024

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:
Prefer tomlkit over toml for building Lightning wheels, and choose tomli and tomllib over toml when installing the package.

Description of the Change:

Benefits:

  • tomli and tomllib (for python > 3.10) provides a read-only, lightweight and fast toml parser compare to toml
  • tomlkit gets regular updates and provides support for all the toml updates and new standards. toml hasn't been updated since Oct 2020.
    Possible Drawbacks:

Related GitHub Issues:
[sc-71707]

@maliasadi maliasadi added the ci:build_wheels Activate wheel building. label Aug 16, 2024
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Thanks Ali

scripts/configure_pyproject_toml.py Outdated Show resolved Hide resolved
.github/workflows/wheel_linux_aarch64.yml Outdated Show resolved Hide resolved
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.50%. Comparing base (89a1bed) to head (2c90785).
Report is 79 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (89a1bed) and HEAD (2c90785). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (89a1bed) HEAD (2c90785)
9 6
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #857       +/-   ##
===========================================
- Coverage   92.20%   59.50%   -32.70%     
===========================================
  Files          93       24       -69     
  Lines       11376     2146     -9230     
===========================================
- Hits        10489     1277     -9212     
+ Misses        887      869       -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maliasadi maliasadi removed the ci:build_wheels Activate wheel building. label Aug 19, 2024
@maliasadi maliasadi added the ci:build_wheels Activate wheel building. label Aug 19, 2024
@vincentmr vincentmr added this to the v0.38.0 milestone Aug 23, 2024
Copy link
Contributor

@vincentmr vincentmr left a comment

Choose a reason for hiding this comment

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

Why do we want to use tomlkit over toml again? Seems like we'll favor tomlkit so could you update the PR's title?

scripts/configure_pyproject_toml.py Outdated Show resolved Hide resolved
@maliasadi
Copy link
Member Author

Why do we want to use tomlkit over toml again? Seems like we'll favor tomlkit so could you update the PR's title?

We initially aimed to transition away from using toml to more efficient, read-only toml parsers like tomli and tomllib (available in Python 3.11+). Both of these parsers offer standardized and lightweight toml parsing, which is more efficient compared to toml. This made them ideal for situations where only reading toml files was necessary. However, moving Lightning to the new build system, we need for modifying toml files, which required a more capable library like tomlkit. Unlike toml, tomlkit offers better support for structured files and is regularly updated to keep up with the latest standards.. I keep the toml logic as it's still a package requirement for PL and a fallback for Windows users.

We now use tomlkit (and toml only on Windows) for building wheels to handles modifications to toml files during the build process. cibuildwheel on Windows has a limited support for tomlkit and as such we are using toml as a fallback. For installation purposes, where only toml parsing is needed, we prefer tomli or tomllib over toml, as they provide a more efficient and standardized solution.

@AmintorDusko
Copy link
Contributor

We now use tomlkit (and toml only on Windows) for building wheels to handles modifications to toml files during the build process. cibuildwheel on Windows has a limited support for tomlkit and as such we are using toml as a fallback.

@maliasadi, why the exception for Windows?

@maliasadi
Copy link
Member Author

why the exception for Windows?

@AmintorDusko cibuildwheel on Windows expects toml polices to parse the pyproject.toml file instead of tomli or tomlkit. It doesn't have any implications on our build or install systems of Lightning.

@maliasadi maliasadi removed this from the v0.38.0 milestone Aug 28, 2024
.github/CHANGELOG.md Outdated Show resolved Hide resolved
@multiphaseCFD
Copy link
Member

Thanks @maliasadi for the nice work. I'm wondering if it works with arm64?

Copy link
Contributor

@vincentmr vincentmr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @maliasadi .

setup.py Show resolved Hide resolved
Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@multiphaseCFD multiphaseCFD left a comment

Choose a reason for hiding this comment

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

Great job!

@maliasadi maliasadi removed the ci:build_wheels Activate wheel building. label Sep 6, 2024
@maliasadi maliasadi merged commit 70dfdf7 into master Sep 9, 2024
74 of 81 checks passed
@maliasadi maliasadi deleted the rm_toml branch September 9, 2024 17:58
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.

6 participants