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

MSC1497: Advertising support of experimental features in the CS API #1497

Closed
ara4n opened this issue Aug 8, 2018 · 24 comments
Closed

MSC1497: Advertising support of experimental features in the CS API #1497

ara4n opened this issue Aug 8, 2018 · 24 comments
Labels
kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal

Comments

@ara4n
Copy link
Member

ara4n commented Aug 8, 2018

Documentation: https://docs.google.com/document/d/10t3Ehz1JZWdDCSAS3SvxZZwb3cXYnJBoOPL4tcIKNYg/edit#heading=h.h8g8a1l2xtks

Sorry, I should have done this as a markdown MSC; consider it the last of a dying breed.

@ara4n ara4n added proposal-ready-for-review proposal A matrix spec change proposal labels Aug 8, 2018
@turt2live
Copy link
Member

Fixes #490 (from the looks of it)

@non-Jedi
Copy link
Contributor

non-Jedi commented Aug 8, 2018

A couple of notes:

  • as far as I know, this will be the first time the spec mentions the "unstable" namespace. Thought needs to be given to whether it's appropriate to include that in the spec and what "unstable" would mean in such cases.
  • Should modules start being versioned separately from the overall spec? This seems like a good idea especially for something like lazy loading that's special-cased a bit on the chat use-case and might need some iterations to get perfect. If we version modules, this seems like the right proposal to introduce the concept with.
  • We should consider a richer set of primitives for module requirements. Specifically, if a module is landing in a minor release of the spec as optional (to avoid violating semver) but is expected to become required in a future version, it's probably a good idea to communicate that somehow.
  • Might it not be a good idea to require servers to advertise all the modules they support (including required ones) for symmetry and in case modules go from required to optional or vice-versa in the future?
  • While versions is being touched, would it be a good idea to add a field with the name and version of the homeserver being run?

EDIT: strikethrough where I'm very wrong.

UNEDIT: (point 1 no longer strikethrough) I don't think I was very wrong; at most slightly wrong.

@turt2live
Copy link
Member

as far as I know, this will be the first time the spec mentions the "unstable" namespace. Thought needs to be given to whether it's appropriate to include that in the spec and what "unstable" would mean in such cases.

Unstable is mentioned throughout the spec. It's even listed as a possible version.

@ara4n
Copy link
Member Author

ara4n commented Aug 8, 2018

as far as I know, this will be the first time the spec mentions the "unstable" namespace. Thought needs to be given to whether it's appropriate to include that in the spec and what "unstable" would mean in such cases.

It slightly surprises me that we don’t mention unstable anywhere, but not that much. Might as well define it now as a prefix for experimental new stuff which hasn’t been versioned and released in the spec yet.

Should modules start being versioned separately from the overall spec?

Probably not, imo, in line with the antigoals section. The point of the monolithic version number of the CS API is to define the set of endpoints (and thus modules) which play form a single well defined release of the spec - even if some of those modules are optional.

This seems like a good idea especially for something like lazy loading that's special-cased a bit on the chat use-case and might need some iterations to get perfect. If we version modules, this seems like the right proposal to introduce the concept with.

If a module’s API needs backwards incompatible changes we’d just bump the overall version for the CS API to reflect it. Plus I’m hoping we might get the LL stuff right before it is baked into a release version number of the CS API.

We should consider a richer set of primitives for module requirements. Specifically, if a module is landing in a minor release of the spec as optional (to avoid violating semver) but is expected to become required in a future version, it's probably a good idea to communicate that somehow.

Hm, this feels quite feature creepy to me...

Might it not be a good idea to require servers to advertise all the modules they support (including required ones) for symmetry and in case modules go from required to optional or vice-versa in the future?

I guess i was trying to keep the response size small and avoid redundant info. After all, optional modules should very much be the exception rather than the norm, and by the time they become compulsory the clients will assume they’re available and not care about the /versions response.

While versions is being touched, would it be a good idea to add a field with the name and version of the homeserver being run?

It’s feature creepy, but i agree it’d be really nice to fix that at last.

@uhoreg
Copy link
Member

uhoreg commented Aug 8, 2018

e2e key backups seem like another possible use for this

@non-Jedi
Copy link
Contributor

non-Jedi commented Aug 8, 2018

I actually really like the idea of having modules be versioned separately from the main spec, and I don't see it at all in the vein of XMPP fragmentation. The idea is more akin to how some PLs are now versioning their language separately from their standard library. We get a solid foundation of functionality from the main matrix spec (the general set of abstractions that matrix uses: rooms, users, events, devices, etc.) and then modules build convenience features on top of that.

Since the modules are by definition not basic functionality of the matrix protocol (this is how it is now, too, not just in the hypothetical future I'm painting), it might be more necessary to iterate faster on the shape of those apis/conventions. Basically, I'm advocating for separate versioning of modules because of the following advantages I see:

  • People working on developing modules aren't held back by the pace of the general spec release timeline. Because modules tend to have less fundamental features ("convenience features"), more iteration is often necessary to get things "right".
  • This keeps the pace of releases of the main spec from speeding up too much (difficult for client or server authors to keep up) since it isn't necessary to increment the major version number for every breaking change to a module.
  • This enables client authors to upgrade piece-meal since they can continue using older versions of modules while upgrading others.

Disadvantages: this requires some sort of negotiation of which version of modules is used by client/server. This requires servers to potentially maintain multiple versions of the same functionality. More care must be given to how modules are written to make sure that they can take part in whatever "module version handshake" is chosen above. This is discussing almost entirely a different proposal than the one written here (the reason I bring it up is that implementation of this proposal begins to preclude versioning of modules).

I definitely see the disadvantages, but I'm really not super content with the current shape of lazy-loading and m.room.message, and I'd prefer not to have to wait until r2 (assuming r1 comes this month) to get those right.

I guess i was trying to keep the response size small and avoid redundant info. After all, optional modules should very much be the exception rather than the norm, and by the time they become compulsory the clients will assume they’re available and not care about the /versions response.

Not including all of the fairly small number of modules because of response size feels like premature optimization to me (different matter if we were talking about sync response of course). I'm really not convinced that we'll believe the same set of modules are required vs optional for all time, and then if we change it in the future, it's a larger burden on old clients to upgrade properly with their feature-checking code.

@non-Jedi
Copy link
Contributor

non-Jedi commented Aug 8, 2018

as far as I know, this will be the first time the spec mentions the "unstable" namespace. Thought needs to be given to whether it's appropriate to include that in the spec and what "unstable" would mean in such cases.

Unstable is mentioned throughout the spec. It's even listed as a possible version.

What I'm getting at I guess is that nowhere on the C2S spec page does it say something like "Endpoints for versions with major version number x will be found under the /_matrix/rx/ path. Endpoints for features being tested but not yet released in a stable spec release should be placed under the /_matrix/unstable/ path. That isn't there yet.

@turt2live
Copy link
Member

What I'm getting at I guess is that nowhere on the C2S spec page does it say something like "Endpoints for versions with major version number x will be found under the /_matrix/rx/ path. Endpoints for features being tested but not yet released in a stable spec release should be placed under the /_matrix/unstable/ path. That isn't there yet.

It is though - it could potentially be made clearer, however the table on the index has a list of the current versions for the all the specs. Each spec then has a list of versions that are available. It looks like we do have a bug where the unstable c2s spec no longer has /unstable in the path - I've filed #1498 for this.

The intention is that by clicking on "unstable" (or HEAD as it's called for some reason) the endpoints reflect what version prefix to use. Likewise, if we ever hit an r1 then clicking on an r0 spec should show r0 for the endpoints.

@non-Jedi
Copy link
Contributor

non-Jedi commented Aug 8, 2018

The intention is that by clicking on "unstable" (or HEAD as it's called for some reason) the endpoints reflect what version prefix to use. Likewise, if we ever hit an r1 then clicking on an r0 spec should show r0 for the endpoints.

Okay that's fine in general (although I'd prefer an explicit statement of the organizing scheme rather than implicit), but endpoints under "unstable" are frequently implemented or changed before a spec PR has even landed to the unstable spec, so I feel like we should have a note somewhere about how endpoints under that prefix may be experimental (or some other similar verbiage).

@turt2live
Copy link
Member

From the MSC:

The client is expected to know how to interpret the compatibility of a given module at different versions. So if a module is announced as available at r0.4.0, but the server also supports r0.5.0 and the client knows about r0.5.0 and knows that the module API is compatible between r0.4.0 and r0.5.0, it should feel free to use it. However, if the client knows the API breaks at r2.0.0, and the server doesn’t explicitly list support for r2.0.0 in its modules response, then the client should consider it unsupported at r2.0.0.

This feels very complex and something that might be better handled by spelling it out rather than having implicit inheritance. With the proposed approach it is easy for a client to assume that r0.x will have lazy loading provided it knows how to speak r0.4 to the server. An older client would then not pick r2 as a viable option anyways because it wouldn't understand it, however a newer client might understand r2. With the client's previous assumption, it would be very possible that the client would try the r0.x lazy loading against r2 if the server didn't advertise it, as the client is trying to figure out how the versioning works.

As mentioned, the solution could be explicitly listing supported versions. For the example scenario, lazy loading would have ['r0.4.0', 'r0.5.0', 'r2.0.0'] even though r0.5 inherits from r0.4, however the client doesn't get an opportunity to make a wrong assumption.

@erikjohnston
Copy link
Member

My main concern with letting module versions drift from spec versions is that there is going to be a lot of cross dependencies between versions of the spec and versions of the modules, in terms of implementing the features.

I really want to avoid a soup of ifdefs on clients where it says things like "if we are on r0.5.0 of main spec and version r0.6.0 on the module, then we can do it like this...", and really, its just going to be a pain to implement, test and maintain client side. And it'll be fragile when a new server comes along and has a different mix of module version.

There are very real benefits to having separate versions, but I also feel that we don't want to be bumping versions of modules that much more often than the main spec anyway, for exactly the same reasons.

I vote at the very least that if your server supports an optional module it must support the same version as the latest version of the main spec the server supports, i.e. if your server supports [r0.3.0. r0.4.0], then it must also support r0.4.0 of any optional modules it supports

@ara4n
Copy link
Member Author

ara4n commented Aug 9, 2018

I've updated the googledoc to reflect erik's feedback (and out-of-band feedback from travis & vdh), and propose a simpler solution for now: that we should pause on the question of negotiating optional modules (and thus whether optional modules should need their own version numbers, and what the semantics should be for servers which only implement modules at given versions of the spec), and instead simply advertise feature flags for experimental features which have not yet landed in the spec. Thus there's now a proposal for an unstable_features response to /versions which does just this.

Meanwhile, I think the whole question of how much to ratify "/unstable" as a version or prefix is slightly orthogonal to the question of cap neg, and eitherway can be punted for now in light of the simpler proposal.

@non-Jedi
Copy link
Contributor

non-Jedi commented Aug 9, 2018

I think the "simpler proposal" is a good idea. I'm not sure after having slept on it if separately versioned modules are actually a good idea for matrix, which tells me the general design of "optional" features needs some more thought before given structure in the spec.

@erikjohnston
Copy link
Member

Having a separate unstable_features key works. I'm slightly concerned that it will be abused and features will get released under unstable and there'll be no way of doing versioned changes, e.g. how do you discover that the server supports a newer form of lazy loading?

Three options spring to mind:

  1. Declare that if you reach that this should never happen because they're unstable features and should be stabilised when ready
  2. Have something like dated version, e.g.: "m.lazy_loading": "2018-08-03". The problem with this is that there's no way of supporting older clients that expect an older version (assuming the version strings are opaque), even if the newer version is backwards compatible.
  3. Have things like "m.lazy_loading.v1": true, i.e., make a new entry for each new iteration of the feature the server supports

@ara4n
Copy link
Member Author

ara4n commented Aug 12, 2018

I was thinking of using option 3 if we ever hit that scenario for incompatible new experimental versions of an existing feature.

@turt2live
Copy link
Member

turt2live commented Aug 20, 2018

I vote that this MSC should enter the final comment period with the view of having it be a spec PR next week. Can the Spec Core Team either approve going into FCP or comment with any concerns. Those outside the team with concerns/suggestions are also welcome to raise them.

@ara4n
Copy link
Member Author

ara4n commented Aug 20, 2018

The watered down version that made it into synapse is working fine so far and is pretty uncontroversial. 👍from me.

@turt2live
Copy link
Member

(that synapse PR being a subset of matrix-org/synapse#3589 )

@richvdh
Copy link
Member

richvdh commented Aug 21, 2018

I vote that this MSC should enter the final comment period with the view of having it be a spec PR next week. Can the Spec Core Team either approve going into FCP or comment with any concerns.

👎. Please can the document be cleaned up to clearly show exactly what is going on.

@anoadragon453
Copy link
Member

@richvdh agreed. Shouldn't take too long.

@ara4n
Copy link
Member Author

ara4n commented Aug 27, 2018

I've fixed up the MSC. @richvdh, @anoadragon453 , @erikjohnston, PTAL again.

@turt2live turt2live added the client-server Client-Server API label Sep 5, 2018
@richvdh
Copy link
Member

richvdh commented Dec 11, 2018

I guess this has happened by default, because LL relies on it and it is implemented in synapse and riot.

@richvdh richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed proposal-in-review client-server Client-Server API labels Dec 11, 2018
@richvdh
Copy link
Member

richvdh commented Dec 12, 2018

I'm renaming this to make way for an MSC for a feature which fits the 'capabilities' name better.

@richvdh richvdh changed the title Capabilities support in the CS API Advertising support of experimental features in the CS API Dec 12, 2018
@richvdh richvdh changed the title Advertising support of experimental features in the CS API MSC1497: Advertising support of experimental features in the CS API Dec 13, 2018
@turt2live turt2live added T-Core and removed T-Core labels Dec 24, 2018
turt2live added a commit that referenced this issue Jan 7, 2019
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Jan 7, 2019
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Jan 24, 2019
@turt2live
Copy link
Member

Merged via #1786 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

No branches or pull requests

7 participants