search

deadpixi / contracts

An implementation of contracts for Python.
GNU Lesser General Public License v3.0
342 stars 18 forks source link

added support for contracts on coroutine functions. #8

Closed asmodehn closed 6 years ago

asmodehn commented 6 years ago

Simplest way I found to support contracts on async functions. The wrapper is async, but the decorators are basic functions, running as usual.

deadpixi commented 6 years ago

Hi @asmodehn

Thank you for this. My only concern is that by adding this code, dpcontracts becomes Python 3-only. I know that Python 2 is getting a bit past its sell-by date, but I'd like to try to maintain compatibility with Python 2 until Python 2 is officially EOL'd (Jan 1st, 2020).

The easiest thing I could think of to do here would be to copy dpcontracts.py to dpcontracts3.py and modify it that way, or make it a module with some magic in __init__.py that imports a different concrete implementation based on the current Python version.

Thoughts?

asmodehn commented 6 years ago

Hello,

That is certainly doable, and possibly the simplest way to keep only one package for users. We might need to check what happens for 2< python_version < 3.5, when coroutines were introduced. Are you aiming at supporting old python3 as well ?

Not sure when I'll have the time to look into it though, but if I do I'll post it here.

On Thu, Aug 23, 2018, 17:13 Rob King notifications@github.com wrote:

Hi @asmodehn https://github.com/asmodehn

Thank you for this. My only concern is that by adding this code, dpcontracts becomes Python 3-only. I know that Python 2 is getting a bit past its sell-by date, but I'd like to try to maintain compatibility with Python 2 until Python 2 is officially EOL'd (Jan 1st, 2020).

The easiest thing I could think of to do here would be to copy dpcontracts.py to dpcontracts3.py and modify it that way, or make it a module with some magic in init.py that imports a different concrete implementation based on the current Python version.

Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/deadpixi/contracts/pull/8#issuecomment-415475667, or mute the thread https://github.com/notifications/unsubscribe-auth/AANgSGwg14X9ya3zD_ZczUivnw7dZ4RFks5uTtTBgaJpZM4VqwR5 .

Zac-HD commented 6 years ago

Either conditionally importing different versions, or using eval(...) in key places to dodge the SyntaxError both sound reasonable to me. Fundamentally there's just no non-ugly way to put async functions in a codebase that also has to run under Python 2... roll on 2020!

asmodehn commented 6 years ago

I came up with this package structure. definitely more complex than just one module... I treated python <3.5 just like python2. Anyway python 3.4 is also nearing end of life. Also, I added tox and travis build to test multiple python versions. Let me know what you think.

Also we probably want to review how we do documentation and testing. I m not quite sure if the python3 part of the doc is actually tested in python3, given that it's dynamic, and I m not quite sure how doctest discover the docs to test...

Zac-HD commented 6 years ago

I'm now wondering if a simpler form is possible, because the single-file approach has a lot to recommend it (not least, users can just copy the file into their project and avoid packaging toolchains). I see two ways to do this:

  1. Use eval(...) wherever we try something with async or await which is a syntax error on earlier Python versions. Ugly but simple.
  2. Use new syntax natively, and the python_requires metadata to block installation on older Pythons. Clean, but leaves Python <=3.4 stuck on dpcontracts==0.4.

I actually prefer (2), because the API is actually really stable, and it's unlikely to change significantly before March 2019 (3.4 EOL), or have Py2-compatible features added before 2020-01-01 (2.7 EOL). Ultimately this is a decision for @deadpixi though!

asmodehn commented 6 years ago

I agree one module is better, since one of the strength of a package dealing with tests (and therefore contracts) is in its simplicity (otherwise you have to test intensively a package designed to test other package...)

  1. I am not a big fan of the eval approach either, as it would likely complexify the code. it is actually equivalent to a conditional import... in one file :-) Might be worth a shot tho, KISS.
  2. Would be nice, but that python_requires feature is quite recent and users on old python envs would be able to install the package, which would end up breaking on async syntax for them.

I would think 1 is better for now, while good and simple python2 compatibility is required. 2 would be better later down the line, after python2 is put on a programming language museum shelf.

Might try 1 to see how it would look like ...

On Sun, Aug 26, 2018 at 2:06 PM Zac Hatfield-Dodds notifications@github.com wrote:

I'm now wondering if a simpler form is possible, because the single-file approach has a lot to recommend it (not least, users can just copy the file into their project and avoid packaging toolchains). I see two ways to do this:

  1. Use eval(...) wherever we try something with async or await which is a syntax error on earlier Python versions. Ugly but simple.
  2. Use new syntax natively, and the python_requires metadata https://packaging.python.org/guides/distributing-packages-using-setuptools/#python-requires to block installation on older Pythons. Clean, but leaves Python <=3.4 stuck on dpcontracts==0.4.

I actually prefer (2), because the API is actually really stable, and it's unlikely to change significantly before March 2019 (3.4 EOL), or have Py2-compatible features added before 2020-01-01 (2.7 EOL). Ultimately this is a decision for @deadpixi https://github.com/deadpixi though!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/deadpixi/contracts/pull/8#issuecomment-416037766, or mute the thread https://github.com/notifications/unsubscribe-auth/AANgSL48wrsOZxhzB0CrktYMsR1b4YjMks5uUp1ygaJpZM4VqwR5 .

asmodehn commented 6 years ago

Quick feedback on Strategy 1. You would need to use 'exec()' as the statement is quite complex. Also it is managed as a closure, nested in another function, etc. (because decorators), and it turns out having the locals getting the right value depending on where and how they are called is not trivial, because exec assumes free variables are global or defined inside a class... and neither is our case. If you are curious, here is the code : https://github.com/asmodehn/contracts/blob/eval_py3/dpcontracts.py#L487 but it is not working as expected and I can't seem to get it right or in any way stable enough today...

So, as a conclusion of these investigations,

Zac-HD commented 6 years ago

In order to keep things simple and crystal clear for the user wanting a quick drag'n'drop/copypaste solution, I would go for a python3 only module, with the same decorator API, and let the user import the module that matches his needs. It is simple enough on the user side to check for python interpreter version and import one module or the other.

Sounds good to me!

deadpixi commented 6 years ago

I like the idea of declaring the current dpcontracts module stable and creating dpcontracts2 that has Python 3.5 support. They'd both be on PyPI with the appropriate python_requires metadata for installation; users could pick whichever one they wanted and/or copy'n'paste as needed.

The consensus is definitely correct, IMHO: Python 3 support should take priority over Python 2 support, as Python 2 has been officially deprecated.

Thoughts, @asmodehn and @Zac-HD ?

Zac-HD commented 6 years ago

This sounds good to me, but I think we could still go for a simpler plan and avoid name proliferation - the distinction between contracts and dpcontracts is already enough to remember and communicate to new users!

Basically, we know that Python 2 will be gone in less than 15 months (and 3.4 in just six months), and it's highly unlikely that we'll want to make any further updates that are compatible with Python 2. So why not just publish a new update that requires Python 3.5?

This would allow us to have a super-clean single module solution, and the python-requires metadata will make it seamless for anyone with a reasonable environment. For anyone using a super-old setuptools, we can add an explicit (helpful) error in setup.py, like Hypothesis and Django. We can even keep a python2 branch and issue further releases under 0.4.x with back-ported features, if you want!

Comment on the specifics for @asmodehn - what's the use-case for async functions as pre- and post-condition checks?

Coming from testing Trio code, the idea of having more or less awaits in my code depending on PYTHONOPTIMIZE is scary, because assertions now have side effects and debugging scheduler issues is the worst. If you're making this possible because it's needed that's one thing, but if it's just for completeness I'd really rather leave them out for now! (especially when adding them later is backwards-compatible but removing them is not)

asmodehn commented 6 years ago

@Zac-HD very good point...

So far I have been mostly investigating/playing around, so it 'd be fair to say that my main motive so far was for completeness. Few more things tho :

Async functions as pre/post-conditions were just an idea to be able to gradually do less checks if you need more performance, or more checks when you have suspicious behavior and you want to enforce some things more strictly. Like a "runtime dynamically-adjusting" optimization level. Thinking about it, it will probably make sense to have a "sub-scheduler" for those maybe (No idea which async lib support an event loop inside an event loop at this stage), so that they won't conflict with the async code the user is aware about.

So, tl;dr : I also think we should leave these out at this stage.

deadpixi commented 6 years ago

So here's a proposal, which I think is mirroring what y'all are saying:

0.5.x is maintained as legacy-compatible (that is with versions of Python <= 3.4). Versions >= 0.6.0 are Python 3.5 and later only.

I've made the necessary changes in master and the new python2 branch. The master branch now has support for async def functions as well (but not for async predicates as per the above).

If this works for everyone I'll push new versions to PyPI; 0.5.0 for legacy Python installs and 0.6.0 for Python >= 3.5.

Zac-HD commented 6 years ago

@deadpixi - the proposal sounds perfect to me, and the implementation looks good too - the python2 branch is perfect, and I'll open a small PR with some suggested tweaks to the Python 3 version (minor stuff to give more useful errors on very old installs, and so on).

Unrelated to the code itself, it would be great to include Alex in the contribution history. Fortunately this is as easy as editing a commit message, then force-pushing to get it in place and finally closing this PR and corresponding issue. I'm happy to rebase my upcoming PR after this, obviously :smile:

deadpixi commented 6 years ago

@Zac-HD Excellent point; I modified that commit. (@asmodehn please make sure that looks good to you too.)

If that looks good to everyone, I'll close the PR and push up to PyPI after Zac-HD's changes. Thanks all!

asmodehn commented 6 years ago

@deadpixi Yes that looks good for me. @Zac-HD Thanks a lot for iterating on this.

Ideally, I would also like a tox/travis-ci test setup to make sure the doc keeps working on upcoming python versions. But that's likely something that belongs in another issue. I ll post it with a proposed PR when I get around to it.

deadpixi commented 6 years ago

Manually merged the parts discussed above into master and closing. Thanks everyone.