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
Merged

cython #548

merged 27 commits into from May 30, 2019

Conversation

samuelcolvin
Copy link
Member

@samuelcolvin samuelcolvin 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
Copy link

codecov bot 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
Copy link
Member Author

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
Copy link
Member Author

samuelcolvin commented May 25, 2019

waiting on pypa/cibuildwheel#139

@samuelcolvin
Copy link
Member Author

Okay, for me this is ready to merge and release, (unless @dmontagu wants to work on #553?).

It's available for install via pip install pydantic==0.27a1, please install, test and give feedback here.

I'll leave it for a few days and then assume either it works or no one could be bothered to test it, and merge.

@tiangolo
Copy link
Member

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
Copy link
Member Author

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

@samuelcolvin
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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

@tiangolo
Copy link
Member

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
Copy link
Contributor

Gr1N commented May 26, 2019

Amazing improvement!

@dmontagu
Copy link
Contributor

dmontagu 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
Copy link
Member

@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
Copy link
Contributor

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

* Tests pass

* Fixed ordering

* Some code cleanup

* Every last file cythonized
@samuelcolvin samuelcolvin mentioned this pull request May 29, 2019
@samuelcolvin samuelcolvin merged commit d473f4a into master May 30, 2019
gangefors added a commit to gangefors/pydantic that referenced this pull request May 31, 2019
* upstream/master: (138 commits)
  add 'none-any.whl' to pypi upload (pydantic#564)
  uprev
  update benchmarks (pydantic#563)
  cython (pydantic#548)
  Fix issue with unspecified generic type (pydantic#554)
  Run dataclass' original __post_init__ before validation (pydantic#560)
  try to stop annoying warnings in azure pipeline (pydantic#549)
  azure pipeline failOnStderr: false
  Azure Pipelines - tests for windows (pydantic#538)
  Fix JSON Schema for list, tuple, and set, improving interoperability (pydantic#540)
  uprev.
  Colors (pydantic#516)
  Fix to schema generation for IPv{4,6}{Address,Interface,Network} (pydantic#532)
  Fix __fields_set__ not using alias field names (pydantic#517) (pydantic#518)
  Change return type hint for create_model (pydantic#526)
  Tuple ellipsis (pydantic#512)
  Fix to schema generation for IPvAny{Address,Interface,Network} (pydantic#498) (pydantic#510)
  uprev
  Scheduled monthly dependency update for May (pydantic#499)
  Implement const keyword in Schema. (pydantic#469)
  ...
@sullivancolin
Copy link
Contributor

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
Addresses the issue raised in pydantic#548 related to running non-compiled in IPython
@dmontagu
Copy link
Contributor

dmontagu 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 June 4, 2019 08:49
samuelcolvin pushed a commit that referenced this pull request Jun 4, 2019
* Update main.py

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

* except AttributeError

* update HISTORY.rst
@sullivancolin
Copy link
Contributor

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants