ICU4X 1.3 and ZeroVec/AsULE Postmortem

[sffc]

I wanted to document the outcome of the 1.3 release, problems that occurred during the release, and mitigations for both clients and team members for future releases.

Summary

ICU4X 1.3 was fully released. However, clients attempting to install or upgrade ICU4X over the weekend, while the release was in progress, received errors involving ZeroVec/AsULE reading like the following:

error[E0277]: the trait bound `props::GeneralCategoryGroup: zerovec::ule::AsULE` is not satisfied
   --> /home/sffc/.cargo/registry/src/index.crates.io-6f17d22bba15001f/icu_properties-1.2.0/src/trievalue.rs:216:20
    |
216 | impl TrieValue for GeneralCategoryGroup {
    |                    ^^^^^^^^^^^^^^^^^^^^ the trait `zerovec::ule::AsULE` is not implemented for `props::GeneralCategoryGroup`

Further, these errors still occur for users attempting to install icu version =1.2 in their Cargo.toml files.

Mitigation

Clients who created an ICU4X lockfile prior to the 1.3 release should continue to work. Furthermore, clients who install or upgrade to ICU4X 1.3 should succeed if performed after Monday, September 25.

However, users who attempt to install a pinned version of ICU4X 1.2 may need to perform additional steps. Recommendations:

1. If using ICU4X 1.2, use component crates rather than the metacrate. The metacrate for ICU4X 1.2 may install 1.3 versions of the component crates, which may cause conflicts in the zerovec versions.
2. If still encountering errors, check your Cargo.lock and ensure that all ICU4X crates are 1.2 and zerovec is 0.9.6, not 0.10 or higher. You may need to run cargo update -p ... to force the correct versions to be installed.

However, please note that ICU4X 1.3 does not include a new CLDR release. It should therefore be a lower-impact change to just upgrade to ICU4X 1.3; there should not be a major reason to keep using 1.2.

Sequence of Events

1. On Thursday, September 21, @Manishearth observes that impl Bake for ZeroVec<T> added a new T: Bake bound. After a brief discussion with @sffc, it was decided to move forward with a patch release to zerovec despite the breaking change. This is because breaking changes are disruptive to users not in the Cargo ecosystem so are best avoided, and we believed that the impact would be small because we verified that all ICU4X types used in a ZeroVec already implemented T: Bake. e16ba52
2. After releasing zerovec 0.9.5 on Friday, September 22, @sffc observed that the tutorials-cratesio job had started failing. @sffc found that at least one type used in a ZeroVec, one in the icu_timezone crate, did not implement Bake in the 1.2 release, so allowing Cargo to resolve to 0.9.5 was breaking these clients. After briefly discussing with @Manishearth, it was decided to roll out 0.9.6, rolling back the addition of T: Bake. Another option considered was to release icu_timezone 1.2.1 to add the Bake impl, but this was eliminated because it was not known how many more crates would have the same problem. 6a787bb. @sffc verified that tutorials-cratesio began passing again after 0.9.6 was released.
3. Soon after zerovec 0.9.6 was released, @Manishearth reverted the removal of T: Bake and prepared zerovec 0.10.0 for release. As part of this, @Manishearth introduced a new range bound such that all ICU4X crates will depend on zerovec >=0.9.6 <0.11 so that either 0.9.6 or 0.10.0 can be used with ICU4X 1.3. 5ced07e
4. On Saturday, September 23, @sffc began releasing ICU4X 1.3 component crates. After starting to release, @sffc observed that the AsULE errors began appearing again in tutorials-cratesio. @sffc decided to ignore these errors with the expectation that they go away after finishing the 1.3 release, since the errors most likely originated from incompatible icu_testdata baked data versions. Update ICU component version numbers #4079
5. When attempting to release the first component crate requiring tinystr, @sffc encountered the AsULE error from cargo publish. @sffc investigated and found that tinystr had a dependency on zerovec 0.9 only, so it was not implementing the AsULE trait from zerovec 0.10. To fix this, @sffc released a new version of tinystr with the range dep. 663b738
6. Also, while uploading component crates, @sffc hit a crates.io rate limit, with the following error printed to the terminal: "the remote server responded with an error (status 429 Too Many Requests): You have published too many new crates in a short period of time. Please try again after Sat, 23 Sep 2023 18:54:19 GMT or email [email protected] to have your limit increased." @sffc emailed that address to request a rate limit increase in the future, but in the mean time kept releasing crates but at a much slower rate.
7. The last component crate was finally published on Sunday, September 24. However, tutorials-cratesio was still failing due to icu_testdata not yet being available at 1.3. @robertbastian resolved this by releasing icu_testdata on Monday, September 25. Temporarily add testdata to repo #4091

Learnings

1. It's not clear why T: Bake was necessary to add. Since it seems avoidable, it could have been saved for the ICU4X 2.0 release where we can absorb breaking changes more easily.
2. The alternative of releasing icu_timezone 1.2.1 could have been considered more seriously.
3. Range deps mean that zerovec 0.9 and 0.10 can appear in the same dep tree, causing errors that can be hard to track down. The range dep improves integration with non-Cargo build systems, since the utils and component crates can be updated independently, but it seems to cause additional problems with Cargo build systems because it silently allows two versions of traits like AsULE to co-exist in the dependency tree. Without a range dep, tinystr would have needed to have been version bumped, and then 1.2 would be purely zerovec 0.9, and 1.3 purely zerovec 0.10.
4. The 1.3 release could have been paused and rolled back as soon as the first tutorials-cratesio failure was noticed and verified.
5. Releasing on a weekend was likely an advantage, because the impact to clients was more limited.

[robertbastian]

The range dep improves integration with non-Cargo build systems, since the utils and component crates can be updated independently

Can you clarify? The only case I can see where a range dep is needed is if you want to update component crates before the updated ZV version is available, but I don't see why that is a requirement.

[sffc]

You can upgrade component crates and then switch zerovec 0.9 to 0.10 in a single changelist. Without range deps, you need to add zerovec 0.10 and have both versions in parallel for a little while.

@Manishearth do you have anything to add?

[robertbastian]

So this is only an issue with build systems that don't support more than one major version of a crate.

[Manishearth]

No, it's not about supporting more than one major version of the crate; it's about the amount of things that can and cannot be done in lockstep. The more hard requirements you have the more brittle the upgrade process becomes.

Furthermore, as more people start using zerovec outside of ICU4X, the range dep helps avoid nasty compatibility errors. Where possible I think we should try and do this; though if we need to make major breaking changes I'm not opposed to not doing this.

[robertbastian]

How does the range dep avoid nasty compatibility errors? From what I've seen it produces nasty errors because we might get multiple versions in tree.

I still don't see why

• vendor dependencies, including our utils
• update components

is an undesirable process in non-cargo build systems.

[Manishearth]

Because in a lot of projects "vendor dependencies" is not a single step. For example, in Google3, each major version must be a separate CL and it goes through compliance review.

There are already a lot of things that are forced to be lockstep in an ICU4X update; as we start settling down as a project I would like us to tie less of these things together.

[Manishearth]

It's not clear why T: Bake was necessary to add. Since it seems avoidable, it could have been saved for the ICU4X 2.0 release where we can absorb breaking changes more easily.

Largely as a future compatability thing: eventually we'd like to use this in the Bake impl in some cases and avoid unsafe.

[sffc]

Yes it makes sense for future compatibility, but (1) we aren't actually using the bound anywhere right now, and (2) it's not clear why it was urgent to add it now and not later in another zerovec release since zerovec is not 1.0 yet.

[Manishearth]

Another learning IMO is that we need to get better about understanding the failure status of the tutorials task (I don't fully understand when it's a real failure), and get rid of testdata ASAP so its presence isn't a wrench in the works.

[sffc]

@mhatzl noted another issue in #4088 (comment) which is that if you rely on cargo install to get a Datagen binary, you can't easily do the pinning solution described above.

[Manishearth]

One problem is that cargo does not seem smart enough to always correctly resolve the range dep uniformly, and will often end up resolving both especially when upgrading from an existing lockfile.

I think there are a couple things we can do to mitigate this.

In the long run, I think for zerovec and yoke it would be nice to primarily use them via the icu_provider reexports so that it's a single crate driving these deps.

In the short run, I would like to publish patch releases for every component and util with a direct dep on zerovec that moves it to 0.10.0.

In general I think the ideal process for a breaking change in a util is:

• release version without breakage
• release breaking change
• use range dep in ICU4X release
• make an ICU4X patch release without the range dep (this is the step we're missing)

This gives a nice migration curve. That said, it's a lot of work, and I can understand wanting to forgo this entirely for future util breakages. The policy I'd like to have is "either we do it right (the way above), or we just do a breaking change with no range dep". Thoughts?

And thoughts on performing that step now?

Note that unless we do ~1.3.1 deps, which I don't think we should, we'll still have some potential for mixed crates, but hopefully it will be good enough to not have cargo resolver problems.

[sffc]

I'm fine uploading 1.3.1 without the range dep.

I lean toward changing the intra-icu deps to ~1.3.1.

While this process sounds like it may work, it still has the skewed release issue, where things can break for clients during the time we are playing ping-pong and running slow cargo publish commands, and there is even more time needed now for that step.

[Manishearth]

i think the bulk of the publish is parallelizeable and the risky is quite low overall and easily detected and fixed. We don't have that many clients, and we haven't even published the blog post yet. We could easily do this tomorrow morning.

A slight risk with ~1.3.1 deps is that cargo could end up pulling multiple icu_providers if you're on 1.3.0. But I think it's probably still better overall, both options have resolver risk (far less resolver risk than the current situation though)

(I have an issue to file about revisiting ~ deps given cargo resolver behavior but I'll get into that later)

[Manishearth]

We're executing this plan in #4107

[sffc]

I think we should endeavor to simplify the release process. The proposed steps require a very high level of expertise in Cargo and dep resolution, and it's not something I would feel confident putting in a release operator handbook. With increased complexity comes increased opportunity to do something wrong.

Causing rippling, hard-to-diagnose breakages from a lower-level library in the Cargo build system seems like something worth avoiding, even at the cost of increasing cost to non-cargo clients.

I therefore am leaning toward recommending that when we need to upgrade a util, we should just upgrade the util and not bother with the intermediate range dep release.

[sffc]

Also, part of the recommendation should be to just roll back breaking changes that can be held for a later release without adversely affecting the crate or its clients.

[Manishearth]

Yes, this is in concordance with what I said: the proposed steps are the "right" way to do this; and when that is too complicated or too much work we should not do them.

I'm putting up a high bar for what we do for mitigation: I'm saying that either it is a big enough problem that it warrants these steps, or it is not a huge deal (99% of cases) and we just do the breaking update no big deal. It means if someone really cares about mitigation, they do need to pull in the cargo experts on the team to do the release. It's a high bar.