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

recommend use of a decorator rather than expose_* #292

Closed
delandtj opened this issue Aug 25, 2018 · 19 comments
Closed

recommend use of a decorator rather than expose_* #292

delandtj opened this issue Aug 25, 2018 · 19 comments

Comments

@delandtj
Copy link

rpyc: absolutely awesome.

recommendation: decorator that exposes properties explicitly. this was a technique used in the pyjs jsonrpc service (which hilariously was 20 lines... gosh shock gasp 50 if you wanted remote error handling)

@coldfix
Copy link
Contributor

coldfix commented Aug 26, 2018

Hi, this idea has already been rejected by the original author. For reasons and a handmade decorator that you can define in your own codebase, see #70. Part of the original rationale is gone (no support for py25), so I'm not 100% against it, if you can convince me that the time is now right.

@traverseda
Copy link
Contributor

traverseda commented Aug 27, 2018

The big reason why this would be good is readability. I want to be able to access roughly the same object both locally and remotely. Having both an adTag method and an exposed_adTag method is unpleasant, unless for some reason exposed_adTag was implementing extra functionality.

Rpyc is already pretty "non-pythonic" in places, and the closer you can get to a simple API that makes it feel like it's "part of python", the more support it's likely to get.

@traverseda
Copy link
Contributor

Also, I'm confused by the original rational as well. I'm not too familiar with rpyc's code base, but I'm presuming that rpyc.Service uses metaclasses. Surely the metaclass could have looked at class attributes at run time and set up decorators appropriately.

@coldfix
Copy link
Contributor

coldfix commented Aug 30, 2018

Service currently has no metaclass (altough I'm okay to change that), and yes for service objects the decorator can be made working in a secure way easily (i.e. without depending on __exposed__ attributes at access time).

I think the bigger question is how other classes should be handled. If there is an @rpyc.exposed decorator that works within Service, users might expect it to work within other classes as well, so how will it be handled there? Require class decorator that add corresponding exposed_ methods? Or handle the exposed-ness by attribute lookup at access time?

The second choice makes for a simpler and IMO more pythonic API but is a pretty big change to permission handling, so we would have to consider it carefully..

@traverseda
Copy link
Contributor

traverseda commented Aug 31, 2018

Well, more generally what I'd like to see is rpyc being able to expose multiple capabilities. So, as an example, I'd like to be able to mark some methods as belonging to the readonly set of capabilities, then extend readonly with the write capabilities to get a readWrite object.

In practice, that opens up a whole can of worms. But if we're talking about some pretty big changes to permission handling, I think it's worth talking about.

In particular is handling of return values. If a readonly capability returns an object, should that object automatically be using the readonly capability as well? It opens a bit of a can of worms as far as developer ergonomics goes.

We might be able to leverage pythons type system for a lot of that, but I don't know. Let me know if you've got any thoughts on it, and I'll try to get something a bit more coherent together discussing that.

@traverseda
Copy link
Contributor

So, let's talk about the ergonomics of remote-procedure calls. Keeping in mind that this is all pie-in-the-sky, and that incremental improvements are a lot more important then anything else.

My natural inclination, if you were looking at a class source without any helpers, is that it would look something like

class CapableList():
    class Readonly(rpyc.Capability):
        item=parent.item
        __length__ = parent.length
    class ReadWrite(Readonly):
        append=parent.append

    @Readonly
    def specialAppend(self,value):
         """Process the value before append
         """
         self.append(int(value))

etc

So, if you had a CapableList, you'd call CapableList.Readonly.item(). If you were to do that as a decorator, the decorator would be something like @Readonly. Nesting classes, while doable in python, doesn't imply any sort of relationship between the outer-and-innter classes. While I think that implementation is one of the more readable ways of doing this, I'm not sure it's even viable.

If it is viable, I think separating objects from their capabilities would be a good way to approach that without having users expect it to work with arbitrary classes.

Of course that would also be a pretty big departure from the existing way of doing things, and would break backwards comparability pretty hard. It's also, I think, pretty hard to make it actually work like that.

I'll try to find some time to see if that approach is viable, of course if it's not something you'd be interested in, if you see major problems with the semantics or something, I'd appreciate your speaking up so I don't spend time on it.

@coldfix
Copy link
Contributor

coldfix commented Sep 5, 2018

there were a couple of PRs concerned with permissions/capabilities a while back in which you might be interested, see e.g. #243. I rejected them at the time without even fully reading, because I found the complexity too much (I completely agree about the incremental changes bit you mentioned).

I'm not sure what the shown code snippet should do. What is e.g. parent what is the purpose of each class?

Nesting classes, while doable in python, doesn't imply any sort of relationship between the outer-and-innter classes.

A relationship could be imposed using metaclasses/decorators, so that is not a general issue.

I'll try to find some time to see if that approach is viable, of course if it's not something you'd be interested in, if you see major problems with the semantics or something, I'd appreciate your speaking up so I don't spend time on it.

As I'm not exactly sure what you're suggesting, so I can't really say. In general I would like better permission/capabilities, but I don't like too much additional complexity both in terms of code base and learning curve.

An alternative approach might be to configure your connections to allow public attributes as well as getattr/setattr/delattr, but then pass only wrapper objects that know how to enforce your desired policies to rpyc. These "policy wrappers" would then take care to again wrap all results from attribute accesses/method calls into an appropriate policy wrapper (e.g. based on type of the returned object and/or attribute name). Some advantages of this method:

  • requires no change in rpyc protocol, can already be done on the users side (but of course it would be helpful to provide some predefined wrappers from rpyc)
  • complete freedom to define your own policies as required
  • you can have different policies for different objects of the same class
  • and even different policy views on the same object, e.g. depending on the privilege level of the current connection
  • doesn't clutter your type system with rpyc-specific stuff
  • lets you define policies for objects of builtin types or types from 3rdparty libraries

@comrumino
Copy link
Collaborator

comrumino commented Oct 17, 2018

I stumbled across this while looking for something else but figured I'd post my thoughts.

If a decorator were to be used in place of the exposed_*, then how those methods are found in the source would have to change. I suppose a decorator could define an attribute __exposed__ for each function and if a given function the attribute __exposed__ make the method available as a RPC.

To indulge the tangent on meta-classes

class MetaMyService(type):
    """ `MyService.__metaclass__` is defined and bound to this class. As this class is
    callable, it is used in place of type and modifies the class dictionary before creation.
    """
    def __new__(mcs, name, bases, dict):
        for exposed_name, svc_func in services.get_exposed():  # __iter__ for name and func 
            dict.update({exposed_name: svc_func)})
        return type.__new__(mcs, name, bases, dict)

class MyService(rpyc.Service):
    __metaclass__ = MetaMyService

Back on topic, for the exposed decorator

def exposed(func):
    @functools.wraps(func)
    def nfunc(*args, **kwargs):
        func.__exposed__ = True
        return func(*args, **kwargs)
    return nfunc

It wouldn't hurt to add the exposed decorator as an option and it should be possible to support both without incurring overhead after startup---on construct check hasattr(func, '__exposed__') for each method and rename the method to exposed_*. The strongest argument against this change is name binding collision (although other unrelated libraries do not seem to worry about this).

I think meta-classes are a cleaner solution than either option. Although, for many the learning curve is probably steep.

EDIT:
the decorator could technically modify func.__name__ and add a prefix exposed_ to the name. not sure of this will have unintended consequences in the code base however, such has how the function is looked up. so, I would have to look into the source before deciding upon the least obtuse choice.

@coldfix
Copy link
Contributor

coldfix commented Oct 19, 2018

I'm okay with adding a metaclass for Service and @exposed decorator as above that effectively only supports descendants of the metaclass as a first step, although this might hurt some that would like to use Service in combination with a different metaclass..

@comrumino
Copy link
Collaborator

comrumino commented Oct 21, 2018

Good point. The meta-class might be best left to the developer. The exposed decorator is a fairly easy addition---maybe it wasn't previously added to avoid the added overhead? So, as a result of the pitfalls, maybe it would be best to add the decorator with an overhead note in the documentation and a wiki section on meta-classes.

If that is good with you @coldfix I can work on a two PRs for the above.

@coldfix
Copy link
Contributor

coldfix commented Nov 1, 2018

Go ahead, that would be great. Don't add too much code though!

@traverseda
Copy link
Contributor

Thanks for keeping the momentum going on this, I've been busy lately and haven't had a time to do anything related to this.

@comrumino
Copy link
Collaborator

No problem. Let me know if there are further suggestions.

@LewisGaul
Copy link

This still sounds like a good idea to me - but seems to have stalled?

@comrumino
Copy link
Collaborator

It's still on the radar. For whatever reason, I've avoided getting to the PR I made prior to being the maintainer.

I'll take another look soon.

@DavidAntliff
Copy link

Is this feature likely to be integrated? The PR looks tidy, and it would be a nice-to-have usability improvement.

I'm also curious to know whether the exposed decorator proposed here would be compatible with the @property decorator, to expose properties?

@comrumino
Copy link
Collaborator

It is likely. It is part of the reason that I became the maintainer for RPyC actually. The trick is a couple years back I switched jobs and I no longer use the library on a daily basis. So, I do no put as much time into the project as I used to. The hard part is that prioritizing feature enhancements and bugs in threading and all the other issues. I literally get pulled in 60 different ways. I think I need to just resolve more recent conflicts. But, I could look into the property decorator compatibility. I've been spending quite a bit of time trying to wrap my head around other existing issues (namely threading, but I've come to realize that RPyC doesn't track threading/frame scope for netrefs). I digress.

I'll try to knock it out for the next release.

@comrumino
Copy link
Collaborator

I always gladly accept high quality PRs.

@comrumino
Copy link
Collaborator

comrumino commented Jul 30, 2022

Version 5.2.0 and later will contain this feature

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

No branches or pull requests

6 participants