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

cython #548

Merged
merged 27 commits into from May 30, 2019

Conversation

Projects
None yet
6 participants
@samuelcolvin
Copy link
Owner

commented May 25, 2019

Related issue number

#547

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • setup travis to build manylinux binary
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>
@codecov

This comment has been minimized.

Copy link

commented May 25, 2019

Codecov Report

Merging #548 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #548   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2453   2491   +38     
  Branches      488    497    +9     
=====================================
+ Hits         2453   2491   +38

samuelcolvin added some commits May 25, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

commented May 25, 2019

as of 7ebf254 pydantic is almost 2x faster on the benchmarks compared to master.

@samuelcolvin samuelcolvin changed the title experimenting with cython cython May 25, 2019

samuelcolvin added some commits May 25, 2019

samuelcolvin added some commits May 25, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

commented May 25, 2019

samuelcolvin added some commits May 26, 2019

@tiangolo

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

Awesome! 🎉

I just installed it and ran all the FastAPI tests, they all passed. ✔️

I installed it on Windows and it installs (the non-Cythonized) version correctly. I ran all the FastAPI tests and they passed (except ujson, which is Linux-only). ✔️

The output of installing with:

pip install pydantic==0.27a1

is:

Collecting pydantic==0.27a1
  Downloading https://files.pythonhosted.org/packages/d4/42/e99aa0a6e4b6ea1fbbf729ffa67615348d66c156c353639a8667562c58f6/pydantic-0.27a1.tar.gz (60kB)
Requirement already satisfied: dataclasses>=0.6 in c:\users\maria\.virtualenvs\fastapi-zgjruwkb\lib\site-packages (from pydantic==0.27a1) (0.6)
Building wheels for collected packages: pydantic
  Building wheel for pydantic (setup.py): started
  Building wheel for pydantic (setup.py): finished with status 'done'
  Stored in directory: C:\Users\maria\AppData\Local\Temp\pip-ephem-wheel-cache-z3eqj6kg\wheels\28\db\fd\c5f791b5ffae394421a3bf005afd0f0b9b1364c75ddce40e45
Successfully built pydantic
Installing collected packages: pydantic
Successfully installed pydantic-0.27a1

So, my use case (FastAPI), of keeping it compatible with Windows and macOS is covered 🎉


This is awesome! Great job @samuelcolvin 🎉 🚀 ! (and thanks @dmontagu for the help and input).


Now I just cross fingers we can #520 work 🤞

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

commented May 26, 2019

yes, I will look at #520 before releasing this in case anything big has to change here.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

commented May 26, 2019

@tiangolo, just out of interest since you have a windows machine: could you try installing after installing cython (pip install cython) and see if:

  1. it tries to compile?
  2. it succeeds in compiling?
  3. if so whether it runs ok
@tiangolo

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

I'll try (I'm doing it).

But I'm doing it on my wife's Windows (double booted) machine, without any C-stuff building toolchain.

It's taking quite some time at least (in Windows sometimes it doesn't report logs of what's doing).

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

commented May 26, 2019

thanks, if it's a hassle don't worry, I was just curious.

@tiangolo

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

I left it until now trying to install in Windows, after installing cython.

It just hanged at:

Collecting pydantic==0.27a1
  Downloading https://files.pythonhosted.org/packages/d4/42/e99aa0a6e4b6ea1fbbf729ffa67615348d66c156c353639a8667562c58f6/pydantic-0.27a1.tar.gz (60kB)

Didn't build, didn't error out, until I killed it.

At least we know, it needs the C-stuff toolchain, etc.


Again, not a priority since it does install in Windows, just not with Cython.

@Gr1N

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

Amazing improvement!

@dmontagu

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

@tiangolo is it possible to try a pip install with the -v flag and share where it hangs? (I'd do it myself but I don't have access to a windows machine -- is there an easy way to try this in the cloud?) I don't know if cython issues are typical on windows but it would seem unfortunate if trying to install the package alongside cython just results in a hang

@tiangolo

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

@tiangolo is it possible to try a pip install with the -v flag and share where it hangs? (I'd do it myself but I don't have access to a windows machine -- is there an easy way to try this in the cloud?) I don't know if cython issues are typical on windows but it would seem unfortunate if trying to install the package alongside cython just results in a hang

I'll try once I get to steal the Windows laptop again.

It would be misfortunate indeed, but I don't think it's an easy scenario to get. For that to happen, someone would have to explicitly install cython and not have the environment available to compile stuff with Cython.

I guess if someone installed/needed Cython in his installation, he/she would probably have the tooling working for it.

Still, I'll try with that once I get a chance.

@dgasmith

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

This is awesome! Just as a note for the Conda deploys. Building a binary for each platform is quite straightforward and I am happy to integrate those builds when a release comes out.

Cythonize more files (#553)
* Cythonize more files

* Tests pass

* Fixed ordering

* Some code cleanup

* Every last file cythonized

@samuelcolvin samuelcolvin referenced this pull request May 29, 2019

Open

Custom root types #507

samuelcolvin added some commits May 30, 2019

@samuelcolvin samuelcolvin merged commit d473f4a into master May 30, 2019

7 of 10 checks passed

Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to af26f7f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
samuelcolvin.pydantic Build #20190530.12 succeeded
Details

gangefors added a commit to gangefors/pydantic that referenced this pull request May 31, 2019

Merge remote-tracking branch 'upstream/master'
* upstream/master: (138 commits)
  add 'none-any.whl' to pypi upload (samuelcolvin#564)
  uprev
  update benchmarks (samuelcolvin#563)
  cython (samuelcolvin#548)
  Fix issue with unspecified generic type (samuelcolvin#554)
  Run dataclass' original __post_init__ before validation (samuelcolvin#560)
  try to stop annoying warnings in azure pipeline (samuelcolvin#549)
  azure pipeline failOnStderr: false
  Azure Pipelines - tests for windows (samuelcolvin#538)
  Fix JSON Schema for list, tuple, and set, improving interoperability (samuelcolvin#540)
  uprev.
  Colors (samuelcolvin#516)
  Fix to schema generation for IPv{4,6}{Address,Interface,Network} (samuelcolvin#532)
  Fix __fields_set__ not using alias field names (samuelcolvin#517) (samuelcolvin#518)
  Change return type hint for create_model (samuelcolvin#526)
  Tuple ellipsis (samuelcolvin#512)
  Fix to schema generation for IPvAny{Address,Interface,Network} (samuelcolvin#498) (samuelcolvin#510)
  uprev
  Scheduled monthly dependency update for May (samuelcolvin#499)
  Implement const keyword in Schema. (samuelcolvin#469)
  ...
@sullivancolin

This comment has been minimized.

Copy link

commented Jun 3, 2019

Not sure if this is fixable or is simply a gotcha.

When testing pydantic in a virtualenv without Cython and using the IPython interpreter, I received the following error:

$ ipython
Python 3.7.3 (default, Mar 27 2019, 09:23:15) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.5.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import pydantic                                                                                                                                                                                                              
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-1-45515a945e82> in <module>
----> 1 import pydantic

~/.virtualenvs/project-svfG-b5G/lib/python3.7/site-packages/pydantic/__init__.py in <module>
      1 # flake8: noqa
----> 2 from . import dataclasses
      3 from .class_validators import validator
      4 from .env_settings import BaseSettings
      5 from .error_wrappers import ValidationError

~/.virtualenvs/project-svfG-b5G/lib/python3.7/site-packages/pydantic/dataclasses.py in <module>
      6 from .errors import DataclassTypeError
      7 from .fields import Required
----> 8 from .main import create_model, validate_model
      9 from .utils import AnyType
     10 

~/.virtualenvs/project-svfG-b5G/lib/python3.7/site-packages/pydantic/main.py in <module>
     66     compiled: bool = False
     67 else:  # pragma: no cover
---> 68     compiled = cython.compiled
     69 
     70 

AttributeError: module 'cython' has no attribute 'compiled'

I think the import cython in the try/except/else located here succeeded because of the %%cython magic which was located ~/.ipython/cython/. In the standard python interpreter it works fine and cython is not importable. In IPython, import cython succeeds but does not have the .compiled attribute

Removing the directory ~/.ipython/cython/ fixes the problem, but the directory is recreated whenever The %%cython magic is used, even from a totally separate virtualenv

dmontagu added a commit to dmontagu/pydantic that referenced this pull request Jun 3, 2019

Update main.py
Addresses the issue raised in samuelcolvin#548 related to running non-compiled in IPython
@dmontagu

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@sullivancolin I think #573 should fix this problem pretty safely; it would be great if you could try running that and see if there are any other places where this problem comes up?

@samuelcolvin samuelcolvin deleted the cython branch Jun 4, 2019

samuelcolvin added a commit that referenced this pull request Jun 4, 2019

Fix Cython compiled check in ipython (#573)
* Update main.py

Addresses the issue raised in #548 related to running non-compiled in IPython

* except AttributeError

* update HISTORY.rst
@sullivancolin

This comment has been minimized.

Copy link

commented Jun 10, 2019

@sullivancolin I think #573 should fix this problem pretty safely; it would be great if you could try running that and see if there are any other places where this problem comes up?

I can confirm that this fixed the issue when Cython is not present and using IPython. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.