web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

Manifest path trie RFC #40

Closed gsnedders closed 4 years ago

gsnedders commented 4 years ago

Rendered: https://github.com/gsnedders/rfcs/blob/manifest-path-trie/rfcs/manifest-path-trie.md.

See also #20 and https://github.com/web-platform-tests/wpt/pull/16537.

jgraham commented 4 years ago

If we have performance numbers for this adding them to the RFC would help make the case for accepting this change.

The details section could mention more clearly the benefits other than file size e.g. the ability to support partial updates.

gsnedders commented 4 years ago

If we have performance numbers for this adding them to the RFC would help make the case for accepting this change.

This RFC itself makes little difference to performance; the big gain is it makes include/exclude much easier to reimplement without iterating through all tests. The only significant change here is it reduces serialization cost, which is primarily apparent when a small number of tests are modified.

The details section could mention more clearly the benefits other than file size e.g. the ability to support partial updates.

This isn't actually any different than before, and I don't think it's changed the implementation complexity of that (reftest-simplification did that).

gsnedders commented 4 years ago

In the case of no substantive disagreement the RFC is considered accepted after 1 week.

tabatkins commented 4 years ago

So yay, I wasn't aware of this change, which caused all of Bikeshed's WPT data to become incorrect and so the entire <wpt> feature to be broken.

I can monitor this repo if necessary, but I wasn't even aware that it existed - it's not, to my knowledge, linked from the README in the main /wpt repo.

Somewhat annoyed that Bikeshed, a known major consumer of the manifest data, wasn't notified of a major change to the manifest format.

Hexcles commented 4 years ago

@tabatkins we're terribly sorry. This really should not have happened. I'm not sure why Bikeshed didn't cross anyone's mind... Especially when I filed https://github.com/web-platform-tests/wpt.fyi/issues/1777 (I even said "all features that rely on /api/manifest are off by default", sigh...)

We are taking steps to prevent this from happening. In addition to the making the RFC repo more prominent from the main WPT repo and including the version number in the response headers of wpt.fyi/api/manifest, I have another idea: https://github.com/web-platform-tests/rfcs/issues/43

jgraham commented 4 years ago

So first of all, apologies for breaking bikeshed; that was obviously unintentional. I had no idea that it was using the manifest, and to be honest didn't even remember that wpt.fyi was providing a public API here.

So clearly one thing that would have improved the situation would be having more consumers pay attention to the RFC repo. But I honestly doubt linking it in the README — though certainly a good idea — is going to make much difference. The RFC process was for example announced on the mailing list, and rather extensively discussed on IRC. It seems unlikely someone who missed all of that would notice a link buried in an already extensive README.

Even if you knew about the RFC repo, it technically doesn't cover wpt.fyi endpoints, and so it would be reasonable to assume that as a consumer of those endpoints changes in the RFC repo don't apply to you.

So in addition to the question about how to make people more aware of RFCs, I think we need to have a clear understanding of what stability guarantees are supposed to be offered by wpt.fyi endpoints, and ensure that there's some mechanism to get notified of changes there. It seems possible that the expectations around stability of the manifest format (largely: that it isn't) are incompatible with the kind of guarantees that wpt.fyi might want to make in general, so perhaps we should only make the raw manifest available on a URL that's explicitly marked unstable, and make other data from the manifest available in a stable format.

tabatkins commented 4 years ago

Apologies accepted, it was a minor hiccup overall and quickly fixed.

The RFC process was for example announced on the mailing list,

I just did a search thru my email for "wpt rfc" and didn't see any relevant hits. When/where did this discussion happen?

and rather extensively discussed on IRC.

w3.org#testing? That channel is extremely noisy with BitBot announcements; if I catch any discussion at all I consider myself lucky. Right now, my 30' vertical-oriented screen shows messages in that room up to 2am this morning (~11 hours ago)

It seems unlikely someone who missed all of that would notice a link buried in an already extensive README.

Strong disagree. There's a nice list of links already in the README with explanations of why you might want to visit each one; having one linking to /rfcs/ and explaining that breaking changes get announced/discussed there would have been caught by me, at least.

tabatkins commented 4 years ago

(That said, I wouldn't mind a more dedicated mailing list for this kind of thing.)

foolip commented 4 years ago

The list where this was discussed is public-test-infra: https://lists.w3.org/Archives/Public/public-test-infra/2019JanMar/0002.html

However, I'm not surprised that not everyone who could be affected by changes to WPT are on that list. Anyway, looks like things got sorted pretty quickly here, and https://github.com/web-platform-tests/wpt/pull/21682 updated the README.

tabatkins commented 4 years ago

Yup, I wasn't subscribed to that; I am now.

And yeah, the README change is good; I consider my issue dealt with. Thanks, all!