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

Documentation on reducing package size and custom CFLAGS #2517

Merged
merged 1 commit into from Jun 4, 2021
Merged

Documentation on reducing package size and custom CFLAGS #2517

merged 1 commit into from Jun 4, 2021

Conversation

peterroelants
Copy link
Contributor

@peterroelants peterroelants commented Mar 13, 2021

Change Summary

  • Documentation update on how to custom compile pydantic when using pip install.
  • Small change in setup.py to allow for custom CFLAGS when compiling.

Setup.py CFLAGS

pydantic's setup.py overwrites all custom CFLAGS that a user might pass in when rebuilding pydantic.
This change appends custom CFLAGS to the current CFLAGS used when building. There should not be any change in the current default behaviour.

By appending custom CFLAGS to the end it is possible to overwrite the default optimization flags currently used in pydantic. From the gcc docs:

If you use multiple -O options, with or without level numbers, the last such option is the one that is effective.

Performance impact of setting different CFLAGS

size benchmark avg time
default pip install (from PyPI) 45M 70.7μs
pip install --no-binary (from PyPI) 796K 101.4μs
pip install --no-binary (from PyPI) with cython 6.4M 67.8μs
pip install --no-binary with cython CFLAG "-O3" (This branch) 6.4M 68.1μs
pip install --no-binary with cython CFLAG "-Os" (This branch) 5.2M 74.6μs

Related issue number

Issue #2276 will be resolved by documenting how to reduce pydantic installation size. Additionally, users can pass in custom CFLAGs to trade-off speed vs size.

Checklist

  • Unit tests for the changes exist (Already existent)
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

Merging #2517 (a2efe5b) into master (7b7e705) will not change coverage.
The diff coverage is n/a.

❗ Current head a2efe5b differs from pull request most recent head d755928. Consider uploading reports for the commit d755928 to get more accurate results

@@            Coverage Diff            @@
##            master     #2517   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5109           
  Branches      1050      1050           
=========================================
  Hits          5109      5109           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aca18a9...d755928. Read the comment docs.

@peterroelants peterroelants changed the title Allow custom CFLAGS and update documentation on reducing size. Documentation on reducing package size and custom CFLAGS Mar 13, 2021
@peterroelants
Copy link
Contributor Author

@PrettyWood I was wondering, should I request a review for this somewhere? And if so, where can I do this?
Or should I just leave it to be reviewed eventually?

@PrettyWood
Copy link
Member

PrettyWood commented Apr 30, 2021

I let Samuel merge PRs. Nothing to do on your side ;)

@samuelcolvin
Copy link
Member

Hopefully I can make time to work on pydantic next week.

@peterroelants
Copy link
Contributor Author

Great, I'll leave it here then. No rush from my side.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

changes/2517-peterroelants.md Outdated Show resolved Hide resolved
docs/install.md Outdated Show resolved Hide resolved
docs/install.md Show resolved Hide resolved
os.environ['CFLAGS'] = '-O3'
# Set CFLAG to all optimizations (-O3)
# Any additional CFLAGS will be appended. Only the last optimization flag will have effect
os.environ['CFLAGS'] = '-O3 ' + os.environ.get('CFLAGS', '')
Copy link
Member

Choose a reason for hiding this comment

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

👍

@samuelcolvin
Copy link
Member

please update.

@peterroelants
Copy link
Contributor Author

peterroelants commented May 9, 2021

please update.

@samuelcolvin Thank you for the review. I updated this PR according to the suggestions, rebased the changes on the latest master, and pushed the changes here. This should be ready to merge. please review.

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Please update and rebase/merge to have new deps to fix CI.
Otherwise looks good thanks!

docs/install.md Outdated Show resolved Hide resolved
@peterroelants
Copy link
Contributor Author

@PrettyWood I:

  • Merged your suggestion,
  • Squashed the commits into a single commit while rebasing on the latest master,
  • Pushed changes to this branch.

Please review

@PrettyWood
Copy link
Member

https://smokeshow.helpmanual.io/4944455v3p5m500e732e/install/

Screen Shot 2021-06-04 at 8 37 38 PM

I think you need to remove the colon at the end of !!! note:
Please update

Squashed commit Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
@peterroelants
Copy link
Contributor Author

https://smokeshow.helpmanual.io/4944455v3p5m500e732e/install/

Screen Shot 2021-06-04 at 8 37 38 PM

I think you need to remove the colon at the end of !!! note:
Please update

resolved

please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants