First draft to show how to make syntactically nicer with type annotations #696

ghandic · 2023-01-08T10:29:53Z

Just the initial example - dont want to go too far down the rabbit hole if there's no appetite. Open to thoughts

Example

class HelloWorldService(ServiceBase):
    @typed_rpc
    def say_hello(
        self,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

Example 2

class HelloWorldService(ServiceBase):
    @typed_rpc(_is_async=True)
    def say_hello(
        self,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

arskom-jenkins · 2023-01-08T10:29:57Z

Can one of the admins verify this patch?

plq · 2023-01-09T08:54:46Z

Hey, this looks neat, i'd definitely merge it if it matures. Thanks for the effort!

Two points:

I can't allow a separate decorator. @rpc should assert that no *params are passed when it detects type-annotations in the function signature and continue reading type information from type annotations instead of *params.
I don't think we support calling @rpc without parens atm. So either the first example should be @rpc() and not @typed_rpc OR we should add another mode of operation that somehow checks that *params == (<member callable?>,) and proceeds as if the decorator is called before decorating (ie @rpc())

What do you think?

ghandic · 2023-01-09T22:00:24Z

So potentially just the first example but blended into the rpc decorator?

Funnily enough, I did that first, but then realized there were many other kwargs I hadn't seen before hence allowing it to have optional pass through kwargs

I think I understand what you mean, but if you could add some dummy examples it would help :)

plq · 2023-01-10T08:45:04Z

The following is OK:

class HelloWorldService(ServiceBase):
    @rpc(_is_async=True)
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

The following should fail with: "*params must be empty when type annotations are used"

class HelloWorldService(ServiceBase):
    @rpc(Unicode, _is_async=True)
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

The following usage of @rpc decorator (no parens) is not supported at the moment but we must make it work. That's 2nd point in my prev post

class HelloWorldService(ServiceBase):
    @rpc
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

The following (with parens) should work same as above:

class HelloWorldService(ServiceBase):
    @rpc()
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

How we handle missing data:

Missing annotations for function arguments : "Missing type annotation for the nth argument"
Missing annotations for function return type: The function does not return anything ie in C-speak it returns void ie just like having _returns omitted ie no error

The following falls under the first point above.

class HelloWorldService(ServiceBase):
    @rpc()
    def say_hello(
        ctx,
        name,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> Iterable(Unicode):
        for _ in range(times):
            yield f"Hello, {name}"

You needn't worry about **kparams, just pass them along.

plq · 2023-01-10T08:46:45Z

The following should fail with: "_returns must be omitted when type annotations are used. Please annotate the return type"

class HelloWorldService(ServiceBase):
    @rpc(_returns=Iterable(Unicode), _is_async=True)
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ):
        for _ in range(times):
            yield f"Hello, {name}"

ghandic · 2023-01-10T10:13:12Z

I've added this exception handling, will try to add some tests

ghandic · 2023-01-10T10:35:39Z

I've blindly added some tests :D couldnt seem to get the run_tests.sh running on my machine - Is the jenkins pipeline visible? - I can only see the last run on Jenkins was 11 months ago? Was there a reason for not using github runners?

plq · 2023-01-10T11:00:51Z

run_tests.sh is quite involved -- it starts by downloading and compiling the desired cpython version, so requires all C tooling to be ready to go. The good news is you don't need it, just switch to your local virtualenv run the tests directly.

There is a readme for tests under spyne/test. It was embarrassingly out of date but I just updated it. Let me know if you still have questions.

Was there a reason for not using github runners?

Spyne project predates those niceties by at least a decade :)

ghandic · 2023-01-10T11:08:57Z

Hmmm python setup.py test

/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/setuptools/dist.py:487: UserWarning: Normalizing '2.15.0-alpha' to '2.15.0a0'
  warnings.warn(tmpl.format(**locals()))
running test
WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.
running egg_info
writing spyne.egg-info/PKG-INFO
writing dependency_links to spyne.egg-info/dependency_links.txt
writing entry points to spyne.egg-info/entry_points.txt
writing requirements to spyne.egg-info/requires.txt
writing top-level names to spyne.egg-info/top_level.txt
reading manifest file 'spyne.egg-info/SOURCES.txt'
adding license file 'LICENSE'
writing manifest file 'spyne.egg-info/SOURCES.txt'
running build_ext
Test stage 1: Unit tests
Traceback (most recent call last):
  File "/Users/andrewchallis/Documents/jsf/spyne2/spyne/setup.py", line 261, in <module>
    setup(
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/setuptools/__init__.py", line 153, in setup
    return distutils.core.setup(**attrs)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/setuptools/command/test.py", line 223, in run
    self.run_tests()
  File "/Users/andrewchallis/Documents/jsf/spyne2/spyne/setup.py", line 226, in run_tests
    ret = call_pytest_subprocess(*tests, capture=self.capture) or ret
  File "/Users/andrewchallis/Documents/jsf/spyne2/spyne/setup.py", line 161, in call_pytest_subprocess
    return call_test(pytest.main, args, tests, env)
  File "/Users/andrewchallis/Documents/jsf/spyne2/spyne/setup.py", line 72, in call_test
    p.start()
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/process.py", line 121, in start
    self._popen = self._Popen(self)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/context.py", line 224, in _Popen
    return _default_context.get_context().Process._Popen(process_obj)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/context.py", line 284, in _Popen
    return Popen(process_obj)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/popen_spawn_posix.py", line 32, in __init__
    super().__init__(process_obj)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/popen_fork.py", line 19, in __init__
    self._launch(process_obj)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/popen_spawn_posix.py", line 47, in _launch
    reduction.dump(process_obj, fp)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/reduction.py", line 60, in dump
    ForkingPickler(file, protocol).dump(obj)
AttributeError: Can't pickle local object '_wrapper.<locals>._'
Traceback (most recent call last):                                                                    
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py", line 201, in main
/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py:216: UserWarning: resource_tracker: There appear to be 3 leaked semaphore objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '
/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py:229: UserWarning: resource_tracker: '/mp-tspay7jf': [Errno 2] No such file or directory
  warnings.warn('resource_tracker: %r: %s' % (name, e))
/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py:229: UserWarning: resource_tracker: '/mp-ejx7eias': [Errno 2] No such file or directory
  warnings.warn('resource_tracker: %r: %s' % (name, e))
/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py:229: UserWarning: resource_tracker: '/mp-37d3chxe': [Errno 2] No such file or directory
  warnings.warn('resource_tracker: %r: %s' % (name, e))
    cache[rtype].remove(name)
KeyError: '/mp-tspay7jf'
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py", line 201, in main
    cache[rtype].remove(name)
KeyError: '/mp-ejx7eias'
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/resource_tracker.py", line 201, in main
    cache[rtype].remove(name)
KeyError: '/mp-37d3chxe'

ghandic · 2023-01-10T11:09:50Z

Jenkins seems to fail to get the repo

ERROR: Error fetching remote repo 'origin'

plq · 2023-01-10T14:23:25Z

don't worry about these, just use pytest to run your specific tests

ghandic · 2023-01-10T21:48:58Z

Done ✅ - I needed to delete spyne/test/interop/test_django.py to run the tests as I get this error

ModuleNotFoundError: No module named 'rpctest'

plq · 2023-01-11T09:30:45Z

Done ✅

Looks good so far. Next, you need to fuse the functionality of @typed_rpc into @rpc and remove @typed_rpc from the public api. For example, no tests should use @typed_rpc , you should always use @rpc, even when testing annotated services.

I needed to delete spyne/test/interop/test_django.py to run the tests as I get this error

As said in spyne/test/README.md, You can avoid all that by calling pytest -v spyne/test/test_service.py -- or whatever test module you want to run

ghandic · 2023-01-11T09:44:21Z

So...

This following use the existing rpc functionality and should all be backward compatible

# All existing with no type annos

class HelloWorldService(ServiceBase):
    @rpc(Unicode,UnsignedInteger32, Iterable(Decimal), Iterable(Iterable(Decimal)), _returns=Iterable(Unicode), _is_async=True)
    def say_hello(ctx, name, times, a, b): 
        for _ in range(times):
            yield f"Hello, {name}"

# Support existing peoples code where they have full type annotations

class HelloWorldService(ServiceBase):
    @rpc(Unicode, UnsignedInteger32, Iterable(Decimal), Iterable(Iterable(Decimal)), _returns=Iterable(Unicode), _is_async=True)
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ): # Note - no return defined
        for _ in range(times):
            yield f"Hello, {name}"

# Support existing peoples code where they have partial type annotations

class HelloWorldService(ServiceBase):
    @rpc(Unicode, UnsignedInteger32, Iterable(Decimal), Iterable(Iterable(Decimal)), _returns=Iterable(Unicode), _is_async=True)
    def say_hello(
        ctx,
        name: Unicode,
        times: UnsignedInteger32,
        a,
        b: Any,
    ): # Note - no return defined
        for _ in range(times):
            yield f"Hello, {name}"

BUT, if they dont supply any args to @rpc or, if they mismatch in length from the defined function.... Then we should error?

…ions

plq · 2023-01-11T10:44:51Z

OK I seem to have brought back all functionality lost to bitrot.

plq · 2023-01-11T10:49:55Z

All existing with no type annos

This should work as before

Support existing peoples code where they have full type annotations

The logic to support this will be too complicated, so no need to bother. Users need to choose between having type annotations in function definition or in the @rpc decorator. If type annotations are used, @rpc should raise an exception when it receives positional arguments (*params) OR _returns inside its keyword arguments (**kparams).

Support existing peoples code where they have partial type annotations

Same as above, this isn't supported.

ghandic · 2023-01-11T11:21:15Z

Ok, this next release won't be backward compatible for anyone who had type annotations in their functions?

plq · 2023-01-11T11:29:11Z

Ok, this next release won't be backward compatible for anyone who had type annotations in their functions?

Is this a thing? People have type annotations in their functions decorated by @rpc? Why?

ghandic · 2023-01-11T11:33:03Z

I'm going to say there's a chance people have done - allows for IDE autocomplete and able to be type checked using mypy

Perhaps some kind of depreciation notice, either way we will probably need to tackle the slightly more complex scenarios above

plq · 2023-01-11T11:43:14Z

OK, this is a good point. hmmm.

First, mypy's type annotators are not compatible with spyne's type annotators. So users will have to choose one or the other.

Maybe instead of

If type annotations are used, @rpc should raise an exception when it receives positional arguments (*params) OR _returns inside its keyword arguments (**kparams).

we should implement

If type annotations are used AND the used type annotators are Spyne's type annotators, @rpc should raise an exception when it receives positional arguments (*params) OR _returns inside its keyword arguments (**kparams).

Or, we could make sure Spyne's annotators are compatible with mypy's, though I imagine this would open a huge can of worms so I'm not entirely willing to go down that path.

A third option would be to merge this patch as-is (we'd have to rename @typed_rpc to @arpc which would be an alias for @rpc(_annotated=True)) and leave it to the user.

plq · 2023-01-11T11:49:48Z

Or, we could make sure Spyne's annotators are compatible with mypy's, though I imagine this would open a huge can of worms so I'm not entirely willing to go down that path.

Converting mypy types to spyne types while reading annotations inside @rpc should be doable. But making sure spyne types imitate mypy types enough to be picked up by IDEs and the like would turn out to be a tad hairy, I imagine.

ghandic · 2023-01-11T12:07:54Z

Yes this was the next step after supporting type annotations inline - I played around with this mapping but having read your docs there are suggestions on things like float/double/decimal and I feel like most people will just say float without reading the docs where you suggest using decimal.

I think it might be best going forward to make the breaking change to be honest. So long as it's clear in the versioning and docs. It's an easy change for the user to remove redundant code (if they were type hinting in multiple locations) - they'll be happy for it.

ghandic · 2023-01-20T07:34:48Z

Any further thoughts?

plq · 2023-01-20T09:50:15Z

Sorry, yes, I think we should parse annotations like we originally wanted. If backwards compatibility becomes a problem, we can turn off reading annotations with eg by a new spyne.const.READ_ANNOTATIONS flag

ghandic · 2023-01-21T08:23:33Z

I've given it a go

Moved the old implementation of @rpc to @_rpc
Added in a const, this can be monkey patched by the user if they really want by the below
Added depreciation warnings
Backwards compatible if no annotations are given, will receive depreciation warning asking them to move the types

import spyne.const
spyne.const.READ_ANNOTATIONS = False

# Now import rest of spyne functionality
from spyne import (
    Decimal,
    Iterable,
    ServiceBase,
    UnsignedInteger32,
)
from spyne import rpc


class HelloWorldService(ServiceBase):
    @rpc
    def say_hello(
        ctx,
        name: UnsignedInteger32,
        times: UnsignedInteger32,
        a: Iterable(Decimal),
        b: Iterable(Iterable(Decimal)),
    ) -> None:
        for _ in range(times):
            yield f"Hello, {name}"

plq · 2023-01-27T09:36:07Z

Besides the PEP8 violations, this looks good to me. If you could make sure lines don't exceed 80 characters, I can merge this

ghandic · 2023-01-27T12:37:48Z

Ran through black, but looks like it changed quite a bit to align with PEP8

ghandic · 2023-01-27T12:41:22Z

Might be good to add some doccos too, not sure where the best place to add them is, or what your plan is going forward with doccos - latest doccos still say they're WIP?

plq · 2023-01-27T14:03:37Z

Ran through black, but looks like it changed quite a bit to align with PEP8

Huh, this is a mess. Can't you simply break lines manually?

plq · 2023-01-27T14:04:38Z

Might be good to add some doccos too, not sure where the best place to add them is, or what your plan is going forward with doccos - latest doccos still say they're WIP?

The apidocs are all we have. You are free to add additional material under the docs section though.

ghandic · 2023-01-27T14:28:24Z

Huh, this is a mess. Can't you simply break lines manually?

I can, though I'd reccomend using something like black over your whole repo to align and enforce a standard.

Maybe a seperate PR ?

ghandic · 2023-01-28T02:45:37Z

done ✅

ghandic · 2023-02-22T05:31:05Z

Anything remaining for this?

plq · 2023-02-27T16:15:14Z

Sorry, release crunch at $DAYJOB, going to look at this first thing I find some time

ghandic mentioned this pull request Jan 8, 2023

Modernise to use type annotations rather than @rpc(Unicode, Integer, _returns=Iterable(Unicode)) #659

Open

plq added the Enhancement label Jan 9, 2023

ghandic added 3 commits January 11, 2023 13:34

First draft to show how to make syntactically nicer with type annotat…

db138d0

…ions

Adding exception handling as per request

b8790f1

Adding tests - unsure how to run...

0d2fda3

plq force-pushed the feature/typed-rpc branch 2 times, most recently from 3840c4e to 14c7e04 Compare January 11, 2023 10:36

ghandic added 2 commits January 11, 2023 13:39

Adding addtional info for typed_rpc errors to assit debugging

d0648b2

Tests passing

1b3ff88

plq force-pushed the feature/typed-rpc branch from 14c7e04 to 1b3ff88 Compare January 11, 2023 10:39

ghandic added 2 commits January 21, 2023 19:23

Making @rpc use type annotations by default

31f4e11

Updating tests to reflect change

c22d0ef

manually making line length <=80

0165753

ghandic force-pushed the feature/typed-rpc branch from fee03db to 0165753 Compare January 28, 2023 02:43

plq mentioned this pull request Dec 12, 2023

2.15.0 Stable release #705

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First draft to show how to make syntactically nicer with type annotations #696

First draft to show how to make syntactically nicer with type annotations #696

ghandic commented Jan 8, 2023

arskom-jenkins commented Jan 8, 2023

plq commented Jan 9, 2023 •

edited

ghandic commented Jan 9, 2023

plq commented Jan 10, 2023 •

edited

plq commented Jan 10, 2023

ghandic commented Jan 10, 2023

ghandic commented Jan 10, 2023 •

edited

plq commented Jan 10, 2023

ghandic commented Jan 10, 2023

ghandic commented Jan 10, 2023

plq commented Jan 10, 2023 •

edited

ghandic commented Jan 10, 2023

plq commented Jan 11, 2023

ghandic commented Jan 11, 2023

plq commented Jan 11, 2023

plq commented Jan 11, 2023 •

edited

ghandic commented Jan 11, 2023

plq commented Jan 11, 2023

ghandic commented Jan 11, 2023

plq commented Jan 11, 2023

plq commented Jan 11, 2023

ghandic commented Jan 11, 2023

ghandic commented Jan 20, 2023

plq commented Jan 20, 2023 •

edited

ghandic commented Jan 21, 2023

plq commented Jan 27, 2023

ghandic commented Jan 27, 2023

ghandic commented Jan 27, 2023

plq commented Jan 27, 2023

plq commented Jan 27, 2023

ghandic commented Jan 27, 2023

ghandic commented Jan 28, 2023

ghandic commented Feb 22, 2023

plq commented Feb 27, 2023

First draft to show how to make syntactically nicer with type annotations #696

Are you sure you want to change the base?

First draft to show how to make syntactically nicer with type annotations #696

Conversation

ghandic commented Jan 8, 2023

arskom-jenkins commented Jan 8, 2023

plq commented Jan 9, 2023 • edited

ghandic commented Jan 9, 2023

plq commented Jan 10, 2023 • edited

plq commented Jan 10, 2023

ghandic commented Jan 10, 2023

ghandic commented Jan 10, 2023 • edited

plq commented Jan 10, 2023

ghandic commented Jan 10, 2023

ghandic commented Jan 10, 2023

plq commented Jan 10, 2023 • edited

ghandic commented Jan 10, 2023

plq commented Jan 11, 2023

ghandic commented Jan 11, 2023

plq commented Jan 11, 2023

plq commented Jan 11, 2023 • edited

ghandic commented Jan 11, 2023

plq commented Jan 11, 2023

ghandic commented Jan 11, 2023

plq commented Jan 11, 2023

plq commented Jan 11, 2023

ghandic commented Jan 11, 2023

ghandic commented Jan 20, 2023

plq commented Jan 20, 2023 • edited

ghandic commented Jan 21, 2023

plq commented Jan 27, 2023

ghandic commented Jan 27, 2023

ghandic commented Jan 27, 2023

plq commented Jan 27, 2023

plq commented Jan 27, 2023

ghandic commented Jan 27, 2023

ghandic commented Jan 28, 2023

ghandic commented Feb 22, 2023

plq commented Feb 27, 2023

plq commented Jan 9, 2023 •

edited

plq commented Jan 10, 2023 •

edited

ghandic commented Jan 10, 2023 •

edited

plq commented Jan 10, 2023 •

edited

plq commented Jan 11, 2023 •

edited

plq commented Jan 20, 2023 •

edited