Closed patrickkwang closed 4 years ago
I think I've identified a functional deficiency of this _optional
system as well. We're trying to define both the server and client sides of an API. Does "optional" mean that it's optional for the server to provide certain functionality (e.g. an asynchronous interface), or optional for a client to support it (e.g. RemoteKnowledgeGraph)? It can't be both.
Although I appreciate the distinction between optional server elements and optional client elements, I don't see why this can't be both. The core must be supported by both. Servers may provide additional functionality, guided by the _optional system, and clients may implement that additional functionality if they wish.
If RemoteKnowledgeGraph were optionally supported (it’s mandatory, at the moment) then a server could choose to return a Message in that form. At that point, the client has no choice and must be able to parse the RemoteKnowledgeGraph format. So if it’s optional for servers, it’s effectively mandatory for clients.
On the other side, if a client chooses to use the optional query_type_id, then all servers had better support it. The alternative would be for clients to treat different servers differently based on their individual specs, which somewhat defeats the purpose.
1) RemoteKnowledgeGraph is an interesting case. I personally think it should be in _optional, not in mandatory. Our client can't handle it. Does your server actually fully implement it? I think it should fall in the case of: one group wanted to implement it, so put it in _optional, so that if any other group wants to implement that kind of functionality, then there's a public precedent. Clearly a case of what is intended for _optional.
2) I think there is a strong case for servers and clients to be aware of what's in _optional and have code to decline ability to handle it if they don't want to. So, I would define "if it’s optional for servers, it’s effectively mandatory for clients" and "then all servers had better support it" as: let's all update our code so that if _optional elements are present, then our client/server software needs to be able to recognize it and explicitly admit to its inability to handle it in a relatively graceful way.
3) "somewhat defeats the purpose": Maybe somewhat, but in practice I think this provides a public way of proposing new functionality. The reality is that we're all (or several of us?) extending the standard to meet our own needs. We've seen this many times. You've done it, we've done it. Rather than just doing this silently, we should do this in an open manner in _optional rather than a silent manner. There have been multiple occasions where we've run across something that can't be expressed or done in the standard, and when we've aired the problem, you've said that you already ran across that and have solved it by silently making a change in your system. If everyone just silently augments the standard to meet there needs, then we end up doing it in different ways (as what happened with /predicates on your side and /entities on our side). So I think the better thing to do is collaboratively work on _optional to express the respective creativity of our teams but do it in a collaborative manner in _optional, so we don't end up implementing more or less the same thing in multiple ways.
In any case, all that windiness to say that I would be fine with finding a more elegant alternative to _optional, but I can't think of one, and I think _optional is a good thing for us all to be creative in the same sandbox. If we collaboratively agree what everyone must implement, then that goes in the main spec. If we collaboratively agree that not everyone needs to truly implement something, then put it in _optional. We could further agree to bind ourselves that all code should be aware of what's in _optional and somewhat gracefully disclaim support for it. As we all come to realize that something in _optional really are necessary, then those features can migrate to the main spec.
There are pieces in _optional that amount to extra response metadata, like many of the informational fields in Message and Result. I think that these could be moved to the non-optional spec. Better documentation (#99) would help implementers to understand the necessarily longer spec, and to quickly identify the most important bits. There are other pieces, like the feedback and request/response storage capabilities that RTX and ROBOKOP provide, that feel orthogonal to the main interoperability goal. This is especially true in the new-look Translator architecture (i.e. KP/ARA/ARS). These I think should be removed from this repo entirely. The tricky pieces are the ones that involve restructuring of the request/response, the one's we've been discussing like query_type_id and RemoteKnowledgeGraph. I think tough decisions need to be made w.r.t these; they cannot be partially supported and either need to be supported universally or removed.
Ultimately, I think I'm in favor of removing the _optional spec entirely, and promoting or removing all of the associated features.
Good. Let's remove it from the main spec. I don't think it it's such a good idea to remove it from _optional if Robokop will continue to use it. If Robokop completely removes it from their web service, then fine.
Rather than try/catch, I was just think more along the lines of also agreeing on query options that would control features like that. e.g. there could be a allow_remote_knowledge_graph={true/false/optional} or something. Clients can specify this option when querying, perhaps with a default of false if unspecified. That way Robokop would only provide a RemoteKnowledgeGraph if the client said true or optional. On the other side, we would update clients to handle the case where there is a RemoteKnowledgeGraph but no KnowledgeGraph. Either they handle it, or provide and error to a user that the server used an unsupported feature that it was requested not to. i.e. build some explicit awareness of the optional features in our tools, even if it is to explicitly not support them.
I agree that moving some more things from _optional to main would be good.
But I would still disagree with removing the _optional sandbox. Adjustments and modifications have always occurred in this rapidly evolving project, and I think having an _optional allows these experimental features to come into the light. Maybe _experimental is better than _optional? Prohibition (removing _optional/_experimental) will just drive experimental features underground where we can't all easily see them. But if everyone else feels _experimental is detrimental to the Translator project, I'm fine to be outvoted.
false
.I'm all for having a public place for exposing experimental API features. I just don't think it should be a special file in the master branch. If we had an experimental
branch here, we could link to it conspicuously from the master README. It would be easier to keep the experimental branch ahead of master since we could just merge in new commits. It would be straightforward to see which features were experimental by diff-ing the branches.
I think that having an experimental
branch and linking to it would be more visible than the separate optional file. I do think that having a sandbox for both the master
and the experimental
branches would be important.
I am fine with moving these things to an experimental branch, although I remain concerned that this is more confusing to most other people.
How would a reasoner convey what version of the schema it is using? Pure "0.9.4" or "0.9.4+experimental"?
Figuring out exactly what a service supports will be tricky. I think it's not as simple as "0.9.4+experimental" because I anticipate people wanting to use just a subset of the experimental features. If each independent feature is associated with a flag/identifier, it should be straightforward for a service to publish a list of the features they support. This feels kludgy because they'd be duplicating information found in their OpenAPI docs, but I also don't want to have to parse OpenAPI docs. Does anyone else have ideas for how to handle this?
Only thing that comes to my mind is to give each experimental feature a name and then we all implement a /features endpoint that provides a list (or map) of the experimental features that are supported by the server. The client could then check it before deciding what to do. You're right that it probably duplicates what could be gleaned by parsing a server's yaml file, but 1000x more convenient. And one could imagine that a service might include a planned feature in the yaml and only advertise it in /features once it is truly ready.
In the opposite direction, which features are present in a Message can probably be automatically discoverable by the receiver, although again for convenience we might have a "features" map at the top level to make it easier to check and decide how to act before delving into the message. I already have code that looks into the message to determine what's there and how to act, so having an explicit features map seems less useful.
I think the /features endpoint makes good sense. It feels more future-proof than asking the clients to infer the servers' capabilities by parsing what they get back. These features could govern not only the structure of a response, but the way in which it's generated. In that scenario, time and cycles could be wasted if the client and server do not sync up ahead-of-time.
Re: implementing this /features endpoint, it's closely tied in with development of service registries, which should also involve the /predicates endpoint. Let's have that discussion in a different thread.
See the discussions in #89 and #92.
One idea is to develop an extension of the OpenAPI spec itself, e.g. by augmenting this. I offer this idea because it would technically solve our problems, but I think it's a bad idea. The specification schema would be basically unreadable and unmaintainable.