Repository navigation
Simplify generated API method implementations #104
Description
Activity
I've thought about this a little bit - I'm not sure there's a really a win in adding more code to the constructor to achieve this.
The reason we ended up with:
req = pubsub_pb2.Topic(name=name) settings = self._defaults['create_topic'].merge(options) api_call = api_callable.create_api_call(self.stub.CreateTopic, settings=settings) return api_call(req, metadata=self._headers)
and not something shorter (which we had before) is because someone reading it can make sense of what it's doing without having to refer to other code.
req = pubsub_pb2.Topic(name=name) return self._create_topic_callable(req, options)
An earlier iteration of the generated python code looked a bit like this, i.e we initially aimed for this kind of terseness. The feedback we received was 'what does this mean? what is _create_topic_callable doing ?'
In the end, we switched to the slightly longer version we have now. Arguably it is easier to understand for someone browsing the code.
The problem is that you can go arbitrarily far in terms of the code that you put into the function. The existing create_api_call already does a bunch that isn't shown here. So, for the status quo, "what does this mean? what is create_api_call doing?" etc. I'm not sure why the merging of defaults & options has a special status in terms of what someone reading the code needs to understand, as opposed to all of the other things that are done in the course of a call.
The last two lines are key:
.... api_call = api_callable.create_api_call(self.stub.CreateTopic, settings=settings) return api_call(req, metadata=self._headers)To any casual reader, it's clear that
- api_call is a function
- create_api_call creates that function
- it's kind of obvious why passing the req and metadata to the api_call makes sense
The problem is that you can go arbitrarily far in terms of the code that you put into the function
That's true, but in this case, that has not happened. As I said, this code was terser in an earlier version and we've made it more verbose to improve readability.
It seems this proposal is about removing code that merges settings from the method bodies, but what's missing from the issue is an explanation of why it's important to do that.
If it's just to reduce the number of lines of generated code in the method bodies at expense of more lines and complex in the constructor, I'm not sure that's a good trade-off. If there's some other factor, it'd be good to clarify that.
- Originally, there was an VKit goal of making the generated method bodies "one-liners", so that as much of the actual logic as possible was contained in GAX. I think this change would be made in that spirit.
- I'm not sure I believe one version or the other is particularly more readable, although it's hard for me to judge having looked at this code so much.
- On the one hand, I think it's somewhat obscure to carry around an object-level attribute (
_defaults) that contains a bunch of different information (CallOptions, BundleOptions, RetryOptions, PageDescriptor, ...). It's also unclear why we special-case metadata when making the API call, when all the other settings are done throughcreate_api_call. - I think the suggested change is nicer in this regard in that it associates these settings to the actual call they modify in the _*callable object.
- On the other hand, as Tim suggested, it's not totally clear what the
_*callableobjects are doing, either. This may just be pushing complexity around.
- On the one hand, I think it's somewhat obscure to carry around an object-level attribute (
If it's not clear that a call is happening, a small tweak could make that clear - add a 'call' method:
req = pubsub_pb2.Topic(name=name) return self._create_topic_callable.call(req, options)Originally, there was an VKit goal of making the generated method bodies "one-liners", so that as much of the actual logic as possible was contained in GAX. I think this change would be made in that spirit.
- the one-liner goal is a nice-to-have
- I agree with having logic contained in the GAX. Pushing that logic into the constructor just leads to a pretty unreadable constructor.
If it's not clear that a call is happening, a small tweak could make that clear - add a 'call' method:
req = pubsub_pb2.Topic(name=name) return self._create_topic_callable.call(req, options)
In python and (also in ruby/nodejs) this does not make the code clearer:
- add .call is not pythonic and IMHO just makes this more confusing, python already has a callable idiom that should be used instead of creating objects with a call() method
- i.e, the code without .call is more readable
Can you confirm @geigerj point, i.e. that the motivation for this change is about trying to achieve the goal of 1-liner method bodies ?
Yes, the goal is to achieve shorter method bodies.
I would like to point out that there is one fewer code statement per api method in the suggested version - the suggested version has the merge performed in the api callable instead of in the generated method body (or constructor, which isn't possible). Thus, this isn't merely moving code around.
Here's how I understand this issue:
- I don't really see how achieving that goal makes real use of the generated libraries any better. If the code-generation resulted in method bodies that are 10+ lines long, then that would be bad and should be fixed, but going from 4 -> 2 does not really adding anything
- IMHO, the proposed change involves a loss of readability, so we're actually making a trade-off
Here's what I recommend:
- before proceeding with any of these changes, please get feedback from the veneer team. In python, they have reviewed the generated code and are already using it. I'd like to see positive confirmation that these changes will make it easier for them.
cc @jmuk
Do we really want the criterion for change at this point to be "input from the gcloud-python team"? It seems to me that small changes for small benefit are still OK as long as they are not prioritized over more important work.
I'm still unconvinced that this change results in a net loss of readability, and it has no impact on the surface, so I'm not sure why another team needs to get involved.
- Yes, 4 -> 2 doesn't add, it technically subtracts :-P I disagree about the impact - I think every line of generated code subtracted is valuable, because of the huge multiplication factor.
- Like Jacob, I think this change doesn't decrease readability, it just changes the abstraction. IMHO, as much work as possible should live in the GAX abstraction.
- Like Jacob, I think that this code is an implementation detail, and I don't see why their input on our implementation detail is necessary, especially given that our code is not being committed to their repository and we are just a dependency (in the scripting languages). I really don't think this change will have any impact on them. If it does impact them, then our surface needs to be improved to give them the information they need to use it properly.
I'm actually on the side of garrett.
- not sure if the line count matters here -- but generally, shorter is better as long as it's readable.
- agree that this does not change the readability.
- since this doesn't change the interface with handwritten veneers, we could decide our own side only.
I have a few notes here:
- For Ruby specific -- rubydoc.info site has 'show code' link for each of the methods, so it might be helpful to bundle a meaningful chunk of code into a single method; this would support @tbetbetbe's point of view
- I am assuming that building a function object is a bit more expensive than others, and the code structure for a method is mostly static -- a page streaming method is always doing some streaming mechanism and does not change to a retryable method. If this is true, I believe it's better to build api_callables first and then reuse them as far as possible.
So I have notes on both sides, but I think former is not so important than others.
- added a commit that references this issue
on May 24, 2016 @tseaver : I talked to @tbetbetbe and he recommended that I get your feedback on this proposal. What do you think?
@garrettjonesgoogle I'm ambivalent about the change: I don't see a particular win in dividing the method up, and as it is generated code, we should care more about clarity of intent than conciseness. If there were significant performance gains (e.g., caching the callable to save construction time), that might outweigh it in my mind.
Also, I don't see how the
merge(options)bit is handled in the new version, but that may just be my unfamiliarity with the code.1 remaining item
@tbetbetbe , as for this statement:
before proceeding with any of these changes, please get feedback from the veneer team.
In python, they have reviewed the generated code and are already using it.
I'd like to see positive confirmation that these changes will make it easier for them.I don't know that we'd necessarily have to demonstrate that the changes will make it easier; isn't it sufficient to demonstrate that it doesn't make it any harder on them? I think that the benefit is for the toolkit team, because less code is generated.
Can we close on this? Say if no one has any continued or new strong objections by EOD Friday, then we proceed with the simplification? @tseaver @tbetbetbe
Alright, no objections - let's proceed with the simplification in all of the languages which were based on the Python pattern. @jmuk @geigerj @bjwatson @shinfan @michaelbausor
I'm okay with this change. I just have one suggestion from a readability perspective.
Rather than:
self._create_topic_callable = api_callable.create(...)Why not do:
self._create_topic = api_callable.create(...)Then the
create_topicfunction can return:return self._create_topic(req, options)This looks more natural and avoids raising questions of what a callable has to do with this. In FP, it's generally not necessary to distinguish between a function object (i.e. Python callable) and an ordinary function.
Sounds good to me.
One other quick thought is to use
requestrather thanreq. The abbreviation is not immediately obvious to me, and probably won't be to third-party devs.I'm fine with proceeding. I have started the change on my fork of toolkit, but still need update GAX.
We can continue this conversation on the PR once I've got it working with GAX.
- added a commit that references this issue
on Jul 19, 2016
Instead of creating an api_call every time there is a call, create an api_callable object for each API method in the constructor, and pass the request and options to that api_callable. For example, existing implementation of publisher_api.create_topic:
Would change to:
New initialization code needs to be added to the constructor:
As for headers, they could be plumbed through
defaultsinstead of passed individually to each api_callable.As for page streaming, the api_callable would need to dynamically determine whether page streaming would be on or off based on the options.