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

[BUILD SYSTEM] Remove zstd #1291

Merged
merged 3 commits into from
Nov 8, 2024
Merged

[BUILD SYSTEM] Remove zstd #1291

merged 3 commits into from
Nov 8, 2024

Conversation

erick-xanadu
Copy link
Contributor

@erick-xanadu erick-xanadu commented Nov 7, 2024

Context: We do not want to ship zstd.

Description of the Change: Removes zstd from LLVM build process.

Benefits: No zstd.

Possible Drawbacks: Possibly our bytecode takes more space.

Related GitHub Issues:

We will need to remove all caches for this change to take into effect.

@erick-xanadu erick-xanadu added the author:build-wheels Run the wheel building workflows on this Pull Request label Nov 7, 2024
@erick-xanadu erick-xanadu marked this pull request as ready for review November 7, 2024 17:39
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch 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

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.95%. Comparing base (75dc517) to head (94e6713).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1291      +/-   ##
==========================================
+ Coverage   96.72%   97.95%   +1.23%     
==========================================
  Files          56       77      +21     
  Lines        6800    11318    +4518     
  Branches      780      981     +201     
==========================================
+ Hits         6577    11087    +4510     
- Misses        173      181       +8     
  Partials       50       50              

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

@erick-xanadu
Copy link
Contributor Author

https://github.com/PennyLaneAI/catalyst/actions/runs/11729023698 these wheels do not contain zstd. The cache must be purged for this change to take effect.

@rmoyard
Copy link
Contributor

rmoyard commented Nov 7, 2024

What was the reason to set it to FORCE_ON?

mlir/Makefile Outdated Show resolved Hide resolved
@erick-xanadu erick-xanadu removed the author:build-wheels Run the wheel building workflows on this Pull Request label Nov 8, 2024
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 Erick. No issue by me.
Do we know the size difference of the wheels with this change?

Copy link
Contributor

@jay-selby jay-selby 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. Thanks Erick for handling this quickly

@erick-xanadu
Copy link
Contributor Author

erick-xanadu commented Nov 8, 2024

Do we know the size difference of the wheels with this change?

@mlxd : Looks like just 1 MB. It could have been more if we removed both zlib and zstd. But keeping zlib makes the difference very small.

@erick-xanadu erick-xanadu merged commit b12e047 into main Nov 8, 2024
69 checks passed
@erick-xanadu erick-xanadu deleted the eochoa/2024-11-07/no-zstd branch November 8, 2024 21:41
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.

4 participants